[Fix][Relax][ONNX] Correct fmod mapping in Mod constant folding - #20170
Open
shoemoney wants to merge 1 commit into
Open
[Fix][Relax][ONNX] Correct fmod mapping in Mod constant folding#20170shoemoney wants to merge 1 commit into
shoemoney wants to merge 1 commit into
Conversation
The Mod converter assigns numpy_op with the two conventions swapped: fmod=0 (integer mod, sign follows divisor) folds with np.fmod, and fmod=1 (C fmod, sign follows dividend) folds with np.mod. The runtime path is correct (floor_mod for fmod=0, mod for fmod=1), so the bug fires exactly when both operands are constants or initializers and BinaryBase.base_impl folds the result at import time: Mod(-5, 3) with fmod=0 bakes R.const(-2) into the graph where onnxruntime returns 1. The test suite could not catch this because the numpy reference in verify_binary_scalar encoded the identical swap, and the only folded value exercised was 4 mod 8, where both conventions agree. Swap the assignments so fmod=0 pairs np.mod with relax.op.floor_mod and fmod=1 pairs np.fmod with relax.op.mod, correct the inverted test reference, and add a folded-constant regression test with negative operands checked against onnxruntime. Merged PR apache#6160 fixed this exact fmod=0/fmod=1 mapping in the old Relay ONNX frontend (issue apache#6106); the Relax frontend reintroduced it in the numpy constant-fold path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Mod converter (python/tvm/relax/frontend/onnx/onnx_frontend.py:730-736 on main) assigns
numpy_opwith the two conventions swapped:fmod=0(integer mod, sign follows divisor per the ONNX spec) folds withnp.fmod, andfmod=1(C fmod, sign follows dividend) folds withnp.mod. The runtime path is correct (floor_modforfmod=0,modforfmod=1), so the bug fires exactly when both operands are constants or initializers andBinaryBase.base_implfolds the result at import time, e.g. shape-arithmetic subgraphs.Mod(-5, 3)withfmod=0bakesR.const(-2)into the graph where onnxruntime returns 1;Mod(-5.5, 3.0)withfmod=1folds to 0.5 where onnxruntime returns -2.5.The suite could not catch this: the numpy reference in
verify_binary_scalar(tests/python/relax/test_frontend_onnx.py:365 on main) encoded the identical swap, and the only folded value exercised was 4 mod 8, where both conventions agree.This is a reintroduction of a fixed bug: #6160 corrected this exact fmod=0/fmod=1 mapping in the old Relay ONNX frontend (issue #6106); the Relax frontend brought it back in the numpy constant-fold path.
Changes:
fmod=0pairsnp.modwithrelax.op.floor_modandfmod=1pairsnp.fmodwithrelax.op.mod.verify_binary_scalar.test_mod_constant_fold_negative_operands: Mod over two constant tensors with negative operands (int32 fmod=0, int32 fmod=1, float32 fmod=1), folded at import and checked against onnxruntime. All three cases fail before the fix and pass after; the full ONNX frontend test file shows no other delta.Not changed: the dead class-level defaults on
Mod(numpy_op = _np.mod/relax_op = relax.op.mod), which_impl_v10always overwrites since ONNX Mod only exists from opset 10.