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

[Bugfix][TVMScript] Handle R.match_cast as last binding in if/else #16562

Merged

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, using R.match_cast as the last binding would produce a segfault, as var_binding->value was used instead of match_cast->value. In addition, because the last binding of each branch was removed, any changes to the struct info resulting from the match cast were silently discarded.

This commit updates the TVMScript parsing of if/else statements to remove the segfault and maintain the struct info changes produced by the R.match_cast.

@Lunderberg Lunderberg force-pushed the tvmscript_handle_ifelse_match_cast branch from 520f54b to c0a7d09 Compare February 13, 2024 22:25
Prior to this commit, using `R.match_cast` as the last binding would
produce a segfault, as `var_binding->value` was used instead of
`match_cast->value`.  In addition, because the last binding of each
branch was removed, any changes to the struct info resulting from the
match cast were silently discarded.

This commit updates the TVMScript parsing of if/else statements to
remove the segfault and maintain the struct info changes produced by
the `R.match_cast`.
@Lunderberg Lunderberg force-pushed the tvmscript_handle_ifelse_match_cast branch from c0a7d09 to af56392 Compare February 14, 2024 02:21
Copy link
Contributor

@slyubomirsky slyubomirsky left a comment

Choose a reason for hiding this comment

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

This is a welcome bugfix to have and it's good to have the regression test. I'm surprised that the previous logic failed.

@Lunderberg
Copy link
Contributor Author

I'm surprised that the previous logic failed.

Yeah, I was pretty surprised at it, too. I think the core issue is the mismatch between the internal representation of a relax::If node and the python-ish TVMScript that represents it. In order to bridge that mismatch, something needs to either be duplicated, or hoisted outside the relax::If, and that hoisting is where the error crept in.

(Because it introduces a new scope, I think the body of a relax::If is closer to a new python function than to a python if/else block. Hypothetically, we could represent it with a new function in TVMScript, but that representation would be ambiguous with a local relax::Function definition.)

I suspect there's some similar edge cases that could be hunted down within a similar space. For example, a relax::MatchCast whose bound value is relax::If, or a relax::If whose output value is not the last binding of the branch. I suspect that those cases would most the relax::MatchCast inside the relax::If, or introduce a trivial binding, but I haven't tested it. At least, those differences should be semantically equivalent, even if they have slightly different IR representations.

@Lunderberg Lunderberg merged commit bd79374 into apache:main Feb 21, 2024
19 checks passed
@Lunderberg Lunderberg deleted the tvmscript_handle_ifelse_match_cast branch February 21, 2024 15:38
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.

None yet

2 participants