Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIR] [Bugfix] Pass the correct block_sref_reuse to Replace #14023

Merged
merged 3 commits into from
Feb 18, 2023

Conversation

quic-sanirudh
Copy link
Contributor

A mismatch between the blocks present in the result vs the blocks passed in block_sref_to_reuse caused the bug mentioned in #13974.

This patch tries to fix that bug by collecting only the blocks that are part of result and also present in the block replacement map new_block_to_old_. Since the scope block is result, only that block and its child blocks would be replaced, and any replaced block would be present in rewriter.new_block_to_old_. Thus, collecting the replaced blocks from among child blocks of result guarantees that the block_sref_reuse would contain all the replaced blocks and that they'll point to the correct block in result thus avoiding the missing SRef error.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Feb 17, 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

A mismatch between the blocks present in the `result` vs the blocks
passed in `block_sref_to_reuse` caused the bug mentioned in apache#13974.

This patch tries to fix that bug by collecting only the blocks that are
part of result and also present in the block replacement map
`new_block_to_old_`. Since the scope block is `result`, only that block
and its child blocks would be replaced, and any replaced block would be
present in `rewriter.new_block_to_old_`. Thus, collecting the replaced
blocks from among child blocks of `result` guarantees that the
`block_sref_reuse` would contain all the replaced blocks and that
they'll point to the correct block in `result` thus avoiding the missing
SRef error.
@quic-sanirudh quic-sanirudh force-pushed the layout_transform_sref_bug branch from fbb30f0 to 832ffdc Compare February 17, 2023 13:14
Copy link
Member

@Hzfengsy Hzfengsy left a comment

Choose a reason for hiding this comment

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

Please add a regression test for it

src/tir/schedule/primitive/layout_transformation.cc Outdated Show resolved Hide resolved
@quic-sanirudh
Copy link
Contributor Author

Please add a regression test for it

Yes, I realized that I did not add the test after creating the PR. I'll update with the test, thanks.

Copy link
Member

@Hzfengsy Hzfengsy left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @quic-sanirudh

@tqchen tqchen merged commit 6f232f9 into apache:main Feb 18, 2023
@quic-sanirudh quic-sanirudh deleted the layout_transform_sref_bug branch February 18, 2023 06:00
yongwww pushed a commit to yongwww/tvm that referenced this pull request Feb 27, 2023
…4023)

* [TIR] [Bugfix] Pass the correct block_sref_reuse to Replace

A mismatch between the blocks present in the `result` vs the blocks
passed in `block_sref_to_reuse` caused the bug mentioned in apache#13974.

This patch tries to fix that bug by collecting only the blocks that are
part of result and also present in the block replacement map
`new_block_to_old_`. Since the scope block is `result`, only that block
and its child blocks would be replaced, and any replaced block would be
present in `rewriter.new_block_to_old_`. Thus, collecting the replaced
blocks from among child blocks of `result` guarantees that the
`block_sref_reuse` would contain all the replaced blocks and that
they'll point to the correct block in `result` thus avoiding the missing
SRef error.
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