[TIR] Refactor division simplification in RewriteSimplifier#18319
[TIR] Refactor division simplification in RewriteSimplifier#18319tqchen merged 7 commits intoapache:mainfrom
Conversation
…onding test This commit removes the specific case for rewriting division by a constant float in the RewriteSimplifier. Additionally, a new test is introduced to verify the behavior of float division simplification, ensuring that the division is correctly handled without the previous rewrite logic.
|
needs to update the testcase and we are good to go |
| assert ana.can_prove_equal(tvm.tir.floormod(expr1, divisor2), 0) | ||
|
|
||
|
|
||
| def test_simplify_float_division(): |
There was a problem hiding this comment.
looks like this pr already includes a related test. Is there anything that needs to be improved? @tqchen
There was a problem hiding this comment.
Sorry, i meant the failed test case in the CI needs to be updated to reflect the new logic. so ci is green https://ci.tlcpack.ai/blue/organizations/jenkins/tvm-unity/detail/PR-18319/1/ there is structural tests that expects the transformed IR likely the failure is due to expected results in previous transformed logic
|
I think we should have a further discussion, because some of the constant‑folding and algebraic rewrites may also introduce similar issues. Examples include:
However, these rewrites are important for optimization and performance, I think we should consider introducing a deterministic switch or similar option that can disable these transformations when strict, this pr should be closed. |
|
thanks @LeiWang1999 , one thing we might be able to do is to restrict some of the operations only to integer expressions, which won't have the issue |
|
please fix the last testcases, likely we only need to change the script as ci indicated, then we are good to go! |
|
seems there is one last testcase, @LeiWang1999 please update, we are close to get it in |
|
the last failed case CMake Error at cmake/modules/CUDA.cmake:89 (message):
Cannot find cudnn_frontend.h, please set USE_CUDNN_FRONTEND to the path of
the cuDNN frontend header
Call Stack (most recent call first):
CMakeLists.txt:457 (include) |
install https://github.com/NVIDIA/cudnn-frontend locally can fix this issue, but: |
|
thanks @LeiWang1999 , looking at the code, perhaps we can raise atol/rtol to 0.3 for now |
|
cc @MasterJH5574 for @tlopex for viz |
For example:
In previous versions, the division would be rewritten as
x * T.float32(1 / 27), which could lead to unexpected behavior.This commit removes the special-case rule for rewriting division by a constant float in
RewriteSimplifier.Additionally, it adds a new test to verify the behavior of float-division simplification, ensuring divisions are preserved as-is instead of being rewritten into multiplications.
Related discussion: https://discuss.tvm.apache.org/t/discuss-is-constant-division-to-multiplication-rewrite-in-tvm-necessary/18615