Fix for assume statement #16664
Fix for assume statement #16664
Conversation
|
@tvm-bot rerun |
| @@ -473,6 +473,7 @@ class ControlFlowGraphBuilder final : public IRVisitorWithAnalyzer { | |||
| void VisitAccess(const BufferAccess& node, BufferTouch::AccessType touch_type, | |||
| PrimExpr known_value_expr) { | |||
| auto& current_block = out_->control_flow_.back(); | |||
| current_block.current_predicate = CurrentScopePredicate(); | |||
There was a problem hiding this comment.
Nitpick: Instead of adding a member variable, can we add this as an additional parameter to current_block.MakeBufferTouch? That way, we avoid accidental re-use of current_predicate from other callsites.
There was a problem hiding this comment.
Thanks! I added an additional parameter to MakeBufferTouch.
| if A[i] == 0: | ||
| A[i] = 42 | ||
|
|
||
| def expected(A: T.Buffer(16, "int32")): |
There was a problem hiding this comment.
Nitpick: Instead of duplicating the same function definition, I'd recommend expected = before. That makes it clear to a reader that the simplification shouldn't cause any changes.
| @@ -1325,6 +1326,34 @@ def expected(A: T.Buffer(16, "int32")): | |||
| A[i] = 42 | |||
|
|
|||
|
|
|||
| class TestAssume(BaseBeforeAfter): | |||
There was a problem hiding this comment.
Nitpick: The T.assume function is tested in other cases as well, but calling this TestAssume implies this is validating all behavior of T.assume. Can we rename the test case to TestAssumeMayContainAdditionalPredicate?
|
Thanks @Lunderberg I will do the above changes recommended! I realized the issues that after this fix the "TestSimplifyUsingPartiallyProvenBufferValueScatter" test is not completing but, "TestSimplifyUsingPartiallyProvenBufferValueGather" test is passing. There does not seem to be an issue while generating the control flow graph but somewhere later after introducing the fix. Can you please point what might be the possible issue? Currently src/tir/transforms/simplify.cc and src/tir/transforms/reduce_branching_through_overcompute.cc pass works with lowered IR but is there any way how we can use it for T.blocks level implementation of the IR? Thank you! |
|
Hello @Lunderberg and @junrushao can you please review the above PR? |
|
Since there is bug in the scatter test which is not yet resolved it might get difficult to upstream this PR. Closing it for now may raise the same PR later. |
The assume statement was not understanding the complete expression present inside. The predicate was not getting appended along with the buffer access. The additional predicate was added to the conditions list but was not appended while creating the buffer touch . This fix helps to resolve it.
The issue is that the "TestSimplifyUsingPartiallyProvenBufferValueScatter" test is not completing after the fix. Rest of the tests are passing. I guess the issue is not with the fix but might be somewhere else.
For reference : #16577
@Lunderberg, @jverma-quic and @quic-sanirudh Can you please review and point to the possible issue with the above test.