Skip to content

Conversation

@Lunderberg
Copy link
Contributor

This commit resolves a flaky test failure that was introduced in #15081. The unit test tests/python/unittest/test_meta_schedule_schedule_rule_mlt_tc.py::test_padded_matmul_relu failed approximately 30% of the time.

The error was due to changes in IterMapRewriter::NormalizeToIterWithOffset. When attempting a simplfication with TryCombineSplitFromSameSource, if the returned Optional<IterSumExpr> is defined, but the args.size() < 1 check fails, then the expr argument is still overwritten, and presumably can cause a later TryFuseIters(expr) to fail.

This commit replaces expr = opt.value(); with auto combined = opt.value();, preserving the original argument.

This commit resolves a flaky test failure that was introduced in
apache#15081.  The unit test
`tests/python/unittest/test_meta_schedule_schedule_rule_mlt_tc.py::test_padded_matmul_relu`
failed approximately 30% of the time.

The error was due to changes in
`IterMapRewriter::NormalizeToIterWithOffset`.  When attempting a
simplfication with `TryCombineSplitFromSameSource`, if the returned
`Optional<IterSumExpr>` is defined, but the `args.size() < 1` check
fails, then the `expr` argument is still overwritten, and presumably
can cause a later `TryFuseIters(expr)` to fail.

This commit replaces `expr = opt.value();` with `auto combined =
opt.value();`, preserving the original argument.
@Lunderberg Lunderberg requested a review from tqchen June 20, 2023 17:27
@tvm-bot
Copy link
Collaborator

tvm-bot commented Jun 20, 2023

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.

Generated by tvm-bot

1 similar comment
@tvm-bot
Copy link
Collaborator

tvm-bot commented Jun 20, 2023

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.

Generated by tvm-bot

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

@Lunderberg
Copy link
Contributor Author

No problem! Once I had it bisected down, it was relatively quick to apply a fix as well.

@Lunderberg
Copy link
Contributor Author

And I spoke too soon. The original "fix" also prevent several simplifications that are needed for index simplification.

@tqchen
Copy link
Member

tqchen commented Jun 20, 2023

Interesting, @Lunderberg can you send the simplification formula around the regression? i can take a look as well. Ideally the thing should be deterministic, and seems right now they are not

@tqchen
Copy link
Member

tqchen commented Jun 20, 2023

I took a look and send in a fix here #15131

@Lunderberg
Copy link
Contributor Author

I'm not sure at the moment which expression is failing to simplify on main. For some of the choices of split factors, it resulted in the buffer C not being recognized as a region cover. But I didn't quite get to the point of narrowing it down to a specific expression before the end of the day.

With the buggy fix, it broke simplification of n * (x//n) + x%n => x. Closing this PR in favor of your fix.

@Lunderberg Lunderberg closed this Jun 21, 2023
@Lunderberg Lunderberg deleted the arith_affine_bugfix branch June 21, 2023 13:21
@Lunderberg
Copy link
Contributor Author

For complete-ness, it looks like the failure can be traced back to the call to EstimateRegionLowerBound that occurs here, which returns NullOpt with the following arguments.

x = ax0_ax1_ax3_ax4_ax5_fused
y = ax0_0_0_ax1_0_0_fused

var_dom = {x: T.Range(0, 2048)}
indices = [x // 1024, y * 2 + x % 1024 // 512, ax2, x % 512 // 256, x % 256 // 16, x % 16]
predicate = ((x // 1024 * 64 + ax2 * 16 + x % 256 // 16 < 127) and
             (y * 64 + x % 1024 // 512 * 32 + x % 512 // 256 * 16 + x % 16 < 127))

@tqchen
Copy link
Member

tqchen commented Jun 21, 2023

Might be useful for us to send a regression

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.

4 participants