[Arith] Restrict floormod coefficient reduction to keep DetectIterMapstable#19832
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restricts certain floormod rewrite rules in RewriteSimplifier to only apply when c1 % c2 == 0, and updates the corresponding tests. The reviewer suggests further simplifying the target expressions in both modified rules directly to floormod(y, c2) and floormod(x, c2) respectively, as floormod(c1, c2) always evaluates to 0 under the new conditions, which avoids unnecessary recursive simplification overhead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| TVM_TRY_REWRITE_IF(floormod(x * c1 + y, c2), floormod(x * floormod(c1, c2) + y, c2), | ||
| c2.Eval()->value > 0); | ||
| c2.Eval()->value > 0 && c1.Eval()->value % c2.Eval()->value == 0); |
There was a problem hiding this comment.
Since the rewrite is now restricted to cases where c1 % c2 == 0, floormod(c1, c2) will always evaluate to 0. Therefore, floormod(x * floormod(c1, c2) + y, c2) simplifies directly to floormod(y, c2). We can simplify the target expression directly to floormod(y, c2) to avoid unnecessary recursive simplification steps (such as multiplying by zero and adding zero).
TVM_TRY_REWRITE_IF(floormod(x * c1 + y, c2), floormod(y, c2),
c2.Eval()->value > 0 && c1.Eval()->value % c2.Eval()->value == 0);| TVM_TRY_REWRITE_IF(floormod(x + y * c1, c2), floormod(x + y * floormod(c1, c2), c2), | ||
| c2.Eval()->value > 0); | ||
| c2.Eval()->value > 0 && c1.Eval()->value % c2.Eval()->value == 0); |
There was a problem hiding this comment.
Similarly, since c1 % c2 == 0 implies floormod(c1, c2) == 0, the expression floormod(x + y * floormod(c1, c2), c2) simplifies directly to floormod(x, c2). Rewriting directly to floormod(x, c2) avoids intermediate simplification overhead.
TVM_TRY_REWRITE_IF(floormod(x + y * c1, c2), floormod(x, c2),
c2.Eval()->value > 0 && c1.Eval()->value % c2.Eval()->value == 0);
|
cc @LeiWang1999 |
|
would be good to add short comment about context |
tlopex
force-pushed
the
fix-arith-floormod-itermap-19825
branch
from
6e19711 to
c926502
Compare
… stable The rewrites floormod(x * c1 + y, c2) -> floormod(x * floormod(c1, c2) + y, c2) and its mirror floormod(x + y * c1, c2) reduce the multiplier modulo the divisor. This is algebraically valid in isolation, but when the modulo is paired with the matching floordiv of the same fused index it rewrites only the modulo's source, so the two halves no longer share a visible fused expression and DetectIterMap rejects an otherwise bijective split (e.g. a layout that splits a flat index into lane = flat % 128 and reg = flat // 128). Guard both rewrites with c1 % c2 == 0 so the multiplied term is only dropped when it is a multiple of the divisor, which is iter-map-safe, as suggested in issue apache#19825. Add a rewrite-simplify regression for both operand orderings and an end-to-end DetectIterMap regression.
The 5D GridSample change (apache#19816) landed with a clang-format violation on the structured binding for CheckTensorLayout, which fails the repo-wide pre-commit lint (clang-format v20.1.8). Reformat to satisfy the hook.
tlopex
force-pushed
the
fix-arith-floormod-itermap-19825
branch
from
c926502 to
b14a680
Compare
2 participants
This PR fixes #19825, which restricts the rewrites
and
While algebraically valid in isolation, these transformations rewrite only the
floormodside of a matchingfloordiv/floormodpair. As a result, the two expressions no longer share a visible fused index expression, causingDetectIterMapto reject otherwise bijective splits such as:where both expressions originate from the same fused index.
Context
Per the suggestion in #19825, the two rewrites are guarded with
c1 % c2 == 0rather than dropped outright. The multiplied term is still eliminated when it is a multiple of the divisor (e.g.(x*10 + y) % 2 -> y % 2), which is safe forDetectIterMap; only the coefficient-shrinking case (c1not a multiple ofc2) is disabled. Both operand orderings are covered, and the PR adds a rewrite-simplify regression plus an end-to-endDetectIterMapregression test.