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

Lunderberg · 2022-11-15T20:49:28Z

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.

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.

tvm-bot · 2022-11-15T20:49:31Z

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 · 2022-11-15T20:49:31Z

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 · 2022-11-15T20:54:12Z

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

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

junrushao · 2022-11-15T21:06:49Z

python/tvm/script/parser/tir/parser.py

+    elif isinstance(res, (int, bool)):
+        T.evaluate(tvm.tir.const(res))


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

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.

This is a good idea!

junrushao · 2022-11-15T21:07:30Z

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


just curious what's this line for?

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.

Ah I see. Thanks for the clarification!

junrushao · 2022-11-15T21:09:12Z

src/printer/tvmscript_printer.cc

+  // printing we only need to print the value.
  Doc doc;
-  doc << tir_prefix_ << ".evaluate(" << Print(op->value) << ")";
+  doc << Print(op->value);


How about we only sugar CallNode?

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 · 2022-11-15T22:05:56Z

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 · 2022-11-15T22:48:39Z

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

…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

Lunderberg requested a review from junrushao November 15, 2022 20:49

junrushao approved these changes Nov 15, 2022

View reviewed changes

junrushao reviewed Nov 15, 2022

View reviewed changes

Update to print T.evaluate() for readability, except for CallNode

7afacd5

Hzfengsy approved these changes Nov 16, 2022

View reviewed changes

Hzfengsy merged commit 86a5cee into apache:main Nov 16, 2022

Lunderberg deleted the tvmscript_automatic_evaluate branch November 16, 2022 01:29

		elif isinstance(res, (int, bool)):
		T.evaluate(tvm.tir.const(res))

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

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

Uh oh!

Conversation

Lunderberg commented Nov 15, 2022

Uh oh!

tvm-bot commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvm-bot commented Nov 15, 2022

Uh oh!

Lunderberg commented Nov 15, 2022

Uh oh!

junrushao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lunderberg commented Nov 15, 2022

Uh oh!

junrushao commented Nov 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tvm-bot commented Nov 15, 2022 •

edited

Loading