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] Fix PlanAndUpdateBufferAllocationLocation not visiting constant buffer #13605

Merged
merged 2 commits into from
Dec 13, 2022

Conversation

masahi
Copy link
Member

@masahi masahi commented Dec 13, 2022

#13560 is supposed to change only the order in which buffers are visited, but the created buffer_alloc_recorder_ misses buffers associated with AllocateConst nodes. These buffers are not part of func->buffer_map, and appear only in read or match_buffer regions, so they are not caught by existing visitors for BufferLoadNode and BufferStoreNode.

Since constant buffers are not correctly handled by PlanAndUpdateBufferAllocationLocation now, tuning on Hexagon is currently broken with this error. This PR fixes this.

E             File "/home/fparizi/src/tvm_src/src/tir/transforms/plan_update_buffer_allocation_location.cc", line 163
E           TVMError: 
E           ---------------------------------------------------------------
E           An error occurred during the execution of TVM.
E           For more information, please see: https://tvm.apache.org/docs/errors.html
E           ---------------------------------------------------------------
E             Check failed: (buffer_data_to_buffer_.count(source_var)) is false:

Since creating a TVMScript-based test case involving constant buffers (AllocateConst) is difficult, I hope we can merge this simple PR without a test case.

cc @Hzfengsy @FredJia-intellif

@tvm-bot
Copy link
Collaborator

tvm-bot commented Dec 13, 2022

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

@Hzfengsy
Copy link
Member

I'm sorry I missed this when reviewing #13560.

BTW, Could you please add regression tests for Hexagon? Then we can make sure the const tensors can be considered in every PR.

@masahi
Copy link
Member Author

masahi commented Dec 13, 2022

BTW, Could you please add regression tests for Hexagon?

Since we cannot parse a large constant via TVMScript, creating a TVMScript-based test case involving constant buffers (AllocateConst) is difficult or near impossible. But if there is a way to create TensorIR program directly from Relay (using link-params=True to generate AllocateConst), then I can probably create a minimum test case for PlanAndUpdateBufferAllocationLocation.

I think there is no way to create TensorIR from Relay (via CreatePrimFunc) directly in Python currently (everything is contained in TECompiler). But yes, Hexagon tuning keeps breaking due to AllocateConst not handled properly in PRs, so I'll try to fix this situation.

@masahi masahi merged commit 1d98634 into apache:main Dec 13, 2022
@masahi
Copy link
Member Author

masahi commented Dec 13, 2022

Regression test is added in #13606

fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
… buffer (apache#13605)

* Fix PlanAndUpdateBufferAllocationLocation not visiting constant buffer

* add comment
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
… buffer (apache#13605)

* Fix PlanAndUpdateBufferAllocationLocation not visiting constant buffer

* add comment
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.

3 participants