-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
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.
fbb30f0
to
832ffdc
Compare
There was a problem hiding this 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
Yes, I realized that I did not add the test after creating the PR. I'll update with the test, thanks. |
There was a problem hiding this 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
…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.
A mismatch between the blocks present in the
result
vs the blocks passed inblock_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 isresult
, only that block and its child blocks would be replaced, and any replaced block would be present inrewriter.new_block_to_old_
. Thus, collecting the replaced blocks from among child blocks ofresult
guarantees that theblock_sref_reuse
would contain all the replaced blocks and that they'll point to the correct block inresult
thus avoiding the missing SRef error.