-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[Bugfix][TVMScript] Preserve foldable constants while parsing #14318
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] Preserve foldable constants while parsing #14318
Conversation
Avoid folding constants while parsing TVMScript. This bug was introduced as part of the parsing changes implemented in apache#14200. The operator overloads in python delegate to the C++ operator overloads, which perform constant folding. While useful in most circumstances, the script parsing should avoid performing any manipulations while parsing, as this can invalidate unit tests that rely on known TIR inputs. There are a few edge cases that would still be constant-folded while parsing the TVMScript, but these only impact subexpressions that do not contain TVM-specific constructs. For example, `T.evaluate(1 + 2)` would be parsed as `T.evaluate(3)`, because the `1 + 2` subexpression is evaluated during parsing.
|
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 |
2 similar comments
|
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 |
|
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 |
|
@cyx-6 introduced a behavior in the printer where foldable expressions (e.g |
|
Thank you, and that makes sense to use That said, I think this dispatch still makes sense to keep, at least for While the changes to printing could be replicated in the hand-written TVMScript, it's an unexpected change that needs to be remembered, especially when |
|
Hi @Lunderberg Here, we migrate from dispatch to the direct computation based on following concerns:
|
|
The primary reason we are changing this is for consistency with python interface. In python, if we have a = IntImm("int64", 1)
b = IntImm("int64", 2)
print(a + b)then it const-folds to |
|
Thank you @cyx-6 , and those are definitely good reasons. I like the general simplifications, and that especially makes sense to re-use the auto-value promotion.
As a slight correction here, you can also get constant folding between two PrimExpr, such as
Hmm, good point. I had been seeing TVMScript as primarily a representation of the underlying TIR/Relax, rather than a scripting language in itself. As a representation, it should match the underlying TIR/Relax as much as possible and avoid constant folding, whereas a scripting language should produce the best output and perform constant folding. I suppose my key question is: should TVMScript be viewed as a compact representation of TIR/Relax, or as a scripting language? |
|
i think one tradeoff here is that we do introduce some form of sugaring, like meta-programming etc. But we also need to serve some purpose of roundtripping, in which case the printer should detect such cases and operate accordingly. So in that regard, because we already have T.Add which explicitly constructs and do not run constant fold, we do have a tool for roundtripping as well. So we can view operator+ as a magic op that may constant fold, while having the printer to be careful and print out explicit T.Add if there is a desire to not to. |
|
Good point. I had been assuming that the meta-programming was restricted to I think I'm comfortable closing this PR, so long as #14320 can be merged in. The constant-folding makes sense, so long as its effects are purely local. My main confusion as a user was the interaction of constant-folding (this PR) and variable renaming for |
|
Looks like #14320 has been merged (thank you @junrushao !), closing this PR. |
Avoid folding constants while parsing TVMScript. This looks like a bug, which was introduced as part of the parsing changes implemented in #14200.
The operator overloads in python delegate to the C++ operator overloads, which perform constant folding. While useful in most circumstances, the script parsing should avoid performing any manipulations while parsing, as this can invalidate unit tests that rely on known TIR inputs. This wasn't caught by any of the existing unit tests in
test_tvmscript_roundtrip.py, as the inputs to those unit tests are generated by the parsing, and already have their constants folded.There are a few edge cases that would still be constant-folded while parsing the TVMScript, but these only impact subexpressions that do not contain TVM-specific constructs. For example,
T.evaluate(1 + 2)would be parsed asT.evaluate(3), because the1 + 2subexpression is evaluated during parsing.