Skip to content

Conversation

LeiWang1999
Copy link
Contributor

For example:

x = tir.Var("x", "float32")
ry = x / 27
sy = ana.rewrite_simplify(ry)

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

…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.
@tqchen
Copy link
Member

tqchen commented Sep 16, 2025

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():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this pr already includes a related test. Is there anything that needs to be improved? @tqchen

Copy link
Member

@tqchen tqchen Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@LeiWang1999
Copy link
Contributor Author

I think we should have a further discussion, because some of the constant‑folding and algebraic rewrites may also introduce similar issues. Examples include:

  • (x * c1) * c2 → x * (c1 * c2)
  • (x / c1) / c2 → x / (c1 * c2) (with correct division semantics)
  • (x * c1) / c2 simplified if c1 divisible by c2, or vice versa.
  • (x / c1) + c2 normalized into a single division when possible.
  • (y + z) * x → y * x + z * x
  • (x + c1) * c2 → x * c2 + c1 * c2
  • y*x + z*x → (y+z) * x

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.

@tqchen
Copy link
Member

tqchen commented Sep 18, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants