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

[TE] Make elem_offset of the buffers created by te.extern a variable to avoid crash #13297

Merged
merged 3 commits into from
Nov 5, 2022

Conversation

masahi
Copy link
Member

@masahi masahi commented Nov 4, 2022

Currently, when compiling MobileBERT on x86, we get a crash from

if (is_zero(arg->elem_offset)) {
ICHECK(is_zero(value->elem_offset))
<< "Trying to bind a Buffer with offset into one without offset "
<< " required elem_offset=" << arg->elem_offset
<< ", provided elem_offset=" << value->elem_offset;
.

This is caused from the subgraph attached in the test. Since #11341, concatenate is implemented as a TE extern on x86. te.extern(...) doesn't explicitly set the elem_offset parameter of input / output buffers it creates, which ends up elem_offset of these buffers being the default value, 0. This causes a problem from the above check in BindBuffer, since the subgraph does create a non-zero elem_offset slice from a buffer whose elem_offset is set to zero.

Since we don't require buffers created in te.extern(...) to have zero elem_offset, we can avoid this problem by explicitly setting elem_offset to be a variable there. This fix was found by @Lunderberg and I'm sending this PR on behalf of him. He also suggested making the strides parameter an array of Var as well, but I didn't do it to keep the change minimal.

cc @Lunderberg @tkonolige @shtinsa

masahi and others added 2 commits November 5, 2022 05:57
Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
@tvm-bot
Copy link
Collaborator

tvm-bot commented Nov 4, 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.

  • No users to tag found in teams: te See #10317 for details
  • Built docs for commit 3ffe5b1 can be found here.

Generated by tvm-bot

Copy link

@shingjan shingjan left a comment

Choose a reason for hiding this comment

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

LGTM!

@masahi masahi merged commit 56878fa into apache:main Nov 5, 2022
@Lunderberg
Copy link
Contributor

LGTM, and thank you for making this change! For context on the fix, this didn't cause an issue previously, because the default elem_offset=0 could bind to an entire buffer. It only became necessary when the buffer being bound as an argument to the te.extern was a view into a larger backing allocation, which could occur after inlining.

xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 10, 2022
…ble to avoid crash (apache#13297)

* make elem_offset of the buffers created by te.extern a variable

Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>

* add test

* fix te extern create_prim_func test

Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
…ble to avoid crash (apache#13297)

* make elem_offset of the buffers created by te.extern a variable

Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>

* add test

* fix te extern create_prim_func test

Co-authored-by: Eric Lunderberg <elunderberg@octoml.ai>
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.

5 participants