[ARITH][BUGFIX] Fix a bug of iter map floormod(x,2) simplify

[tqchen]

This PR fixes a previous bug introduced in itermap detection.

Specifically, y - (x % 2) were simplified to y + (x % 2) - 1. Which is wrong. The working rule is y + ((x + 1) % 2) - 1, but that rule will change the base iterator which is not desirable here.

[tvm-bot]

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

• cc @junrushao _{See #10317 for details}

_{Generated by tvm-bot}

[tqchen]

cc @Lunderberg @junrushao Please revisit some of the previous floormod(x,2) rules to see f there are similar kinds of problems. cc @wrongtest-intellif

[junrushao]

LGTM! This really took me a while to understand why this rewriting is not correct :-)

[wrongtest-intellif]

The case test_postproc_disallow_async_strided_mem_copy_allows wants to check below index is contiguous but unfortunately failed, I guess it is because rewrite rules in #13936 mutate the sign?

ax0 + 1024 - j % 2 * 1024

{i: T.Range(0, 1024), ax0: T.Range(0, 1024), j: T.Range(0, 1023), ax0: T.Range(0, 1024)}

Currently in iter map we could check 1*a + 1*b could be surjective but could not handle 1*a + -1*b

[tqchen]

That is my guess as well. Would be great to confirm if that is the case. We should prioritize the iter map rewrite and avoid mutate sign of iter map expressions.

[Lunderberg]

Thank you for finding the error there. The rewrite were initially introduced to allow simplification of cases of (x+const)%2 introduced by the InjectSoftwarePipeline pass, with equivalent changes made for DetectIterMap so that it could handle the simplified expressions.

I agree that the DetectIterMap changes are definitely incorrect, and am going through the rewrite rules introduced in that PR to check whether they had a similar error, or whether the error was solely in the IterMapSimplify changes.

[tqchen]

I think the gist is that we should avoid to rewrite (x+1)%2 to 1-x%2 in general. Although this is correct, change positive sign to a negative coefficient will cause issues for iter map detection in general, and is not desirable.

[tqchen]

Update: We also removed the rule that simplifies (x + 1) % 2 => 1 - x % 2
as benefit is minimal and it introduces extra negative coefficients on variables
that hurts analysis in general (as negative coefficients are
harder in many cases).

This hopefully can recover the failed case in test_postproc_disallow_async_strided_mem_copy_allow

The particular rewrites is not that useful in software pipeline because all the is are going to be unrolled

Some take aways

• T0: We should avoid rewriting that generate extra negative coefficients on variables, even it might bring some benefit, negative coefficient makes iterator analysis harder
• T1: We should avoid putting too much burdens on rewrite simplify, some of the examples(that covers long range dependencies) can be better covered by canonical as a result do not need to goto rewrite. Let us check the cases under broader simplifier before adding a new rule for long range dep

[Lunderberg]

Thank you, and that makes sense for now. I'm seeing if I can find the expression that required this simplification step in order to enable a cancellation, but however it gets re-implemented should avoid the breakage in iter map simplification.

[tqchen]

Would also be good to put up such expressions in TVMScript so we have context for new rules

For example, some of the (i +1) %2 expressions do not need simplification mainly because they are in an unrolled loop and the expression will become constant later

[junrushao]

This is merged. Thanks for digging into the analysis!