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.

Sign up for GitHub

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

Jump to bottom

[TVMScript] Use tir::Evaluate if expression is in statement context #13396

Merged
merged 2 commits into from
Nov 16, 2022
Merged

[TVMScript] Use tir::Evaluate if expression is in statement context #13396

merged 2 commits into from
Nov 16, 2022

Conversation

Lunderberg
Copy link
Contributor

For the previous version of the parser, this was special-cased for some intrinsic operators. After the new TVMScript was enabled in #12496, any PrimExpr that appears in the body of a statement is silently ignored. This commit updates the parser to instead wrap the bare PrimExpr in a tir::Evaluate node.

This change effectively allows expression statements in TVMScript, which are converted to tir::Evaluate nodes during parsing.

@Lunderberg
[TVMScript] Use tir::Evaluate if expression is in statement context
35b7464 
For the previous version of the parser, this was special-cased for
some intrinsic operators.  After the new TVMScript was enabled in
apache#12496, any `PrimExpr` that appears
in the body of a statement is silently ignored.  This commit updates
the parser to instead wrap the bare `PrimExpr` in a `tir::Evaluate`
node.

This change effectively allows [expression
statements](https://docs.python.org/3/reference/simple_stmts.html#expression-statements)
in TVMScript, which are converted to `tir::Evaluate` nodes during
parsing.
@Lunderberg Lunderberg requested a review from junrushao November 15, 2022 20:49
@tvm-bot
Copy link
Collaborator

tvm-bot commented Nov 15, 2022 •
edited
Loading

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: tvmscript See #10317 for details
  • Built docs for commit 7afacd5 can be found here.

Generated by tvm-bot

@tvm-bot
Copy link
Collaborator

tvm-bot commented Nov 15, 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: tvmscript See #10317 for details

Generated by tvm-bot

@Lunderberg
Copy link
Contributor Author

This PR is in response to CI failures found in #13130, when writing TVMScript that used the T.assume() builtin without wrapping in T.evaluate(). I am updating that PR to use T.evaluate() in all new unit tests, but wanted to add this as a more general update. With how TVMScript borrows heavily from Python's syntax, neither the pre-#12496 behavior of raising an error for expression statements, nor the post-#12496 behavior of ignoring expression statements felt expected as a user.

junrushao
junrushao approved these changes Nov 15, 2022
View reviewed changes
Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

LGTM! I was thinking about the same thing but holding back a bit because I dont want to change TIR too often, but it is definitely a positive change and I really love it

python/tvm/script/parser/tir/parser.py
Comment on lines +417 to +418
elif isinstance(res, (int, bool)):
T.evaluate(tvm.tir.const(res))
Copy link
Member

Choose a reason for hiding this comment

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

hey i was wondering if we want to explicitly print T.evaluate(0) vs 0? The former one might look a bit clearer from my perspective

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going back and forth on it, and I like the idea of printing T.evaluate() except in the case of CallNode. The more aggressive sugaring would be to render tir::Evaluate(0) as pass, which would be even clearer to read from a casual Python reader.

Copy link
Member

Choose a reason for hiding this comment

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

This is a good idea!

tests/python/unittest/test_tvmscript_roundtrip.py
@T.prim_func
def func(A: T.Buffer[1, "int32"]):
T.evaluate(T.assume(A[0] == 5))
A[0] = 10
Copy link
Member

Choose a reason for hiding this comment

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

just curious what's this line for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because T.assume doesn't have any effect, and I was pulling this in from a failing case of T.assume in a separate PR. This was from a unit test that validated a pass that removed T.assume calls from a primfunc.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. Thanks for the clarification!

junrushao
junrushao reviewed Nov 15, 2022
View reviewed changes
src/printer/tvmscript_printer.cc Outdated
Doc doc;
doc << tir_prefix_ << ".evaluate(" << Print(op->value) << ")";
doc << Print(op->value);
Copy link
Member

Choose a reason for hiding this comment

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

How about we only sugar CallNode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea for the printing. Maybe best of both worlds, to have the sugar apply all the time when parsing, but only CallNode drops the printing of T.evaluate(...) when generating TVMScript.

@Lunderberg
Copy link
Contributor Author

LGTM! I was thinking about the same thing but holding back a bit because I dont want to change TIR too often, but it is definitely a positive change and I really love it

Thank you! It's definitely a lot easier to add parsing/printing sugar to TVMScript than to change the TIR in C++.

@junrushao
Copy link
Member

Thanks for the great update! This helps a lot with the usability of TVMScript :-) Let's get it in ASAP!

@Hzfengsy Hzfengsy merged commit 86a5cee into apache:main Nov 16, 2022
@Lunderberg Lunderberg deleted the tvmscript_automatic_evaluate branch November 16, 2022 01:29
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
@Lunderberg @xinetzone
[TVMScript] Use tir::Evaluate if expression is in statement context (a…
c6226ed 
…pache#13396)

* [TVMScript] Use tir::Evaluate if expression is in statement context

For the previous version of the parser, this was special-cased for
some intrinsic operators.  After the new TVMScript was enabled in
apache#12496, any `PrimExpr` that appears
in the body of a statement is silently ignored.  This commit updates
the parser to instead wrap the bare `PrimExpr` in a `tir::Evaluate`
node.

This change effectively allows [expression
statements](https://docs.python.org/3/reference/simple_stmts.html#expression-statements)
in TVMScript, which are converted to `tir::Evaluate` nodes during
parsing.

* Update to print T.evaluate() for readability, except for CallNode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junrushao junrushao junrushao approved these changes

@Hzfengsy Hzfengsy Hzfengsy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Lunderberg @tvm-bot @junrushao @Hzfengsy