You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/11/15 20:49:30 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Lunderberg opened a new pull request, #13396:
URL: https://github.com/apache/tvm/pull/13396
For the previous version of the parser, this was special-cased for some intrinsic operators. After the new TVMScript was enabled in https://github.com/apache/tvm/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Hzfengsy merged pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
Hzfengsy merged PR #13396:
URL: https://github.com/apache/tvm/pull/13396
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
junrushao commented on code in PR #13396:
URL: https://github.com/apache/tvm/pull/13396#discussion_r1023258542
##########
python/tvm/script/parser/tir/parser.py:
##########
@@ -411,6 +412,10 @@ def visit_expr_stmt(self: Parser, node: doc.Expr) -> None:
if isinstance(res, Frame):
res.add_callback(partial(res.__exit__, None, None, None))
res.__enter__()
+ elif isinstance(res, PrimExpr):
+ T.evaluate(res)
+ elif isinstance(res, (int, bool)):
+ T.evaluate(tvm.tir.const(res))
Review Comment:
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
##########
tests/python/unittest/test_tvmscript_roundtrip.py:
##########
@@ -3458,6 +3458,15 @@ def func() -> None:
return func
+def implicit_evaluate():
+ @T.prim_func
+ def func(A: T.Buffer[1, "int32"]):
+ T.evaluate(T.assume(A[0] == 5))
+ A[0] = 10
Review Comment:
just curious what's this line for?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13396:
URL: https://github.com/apache/tvm/pull/13396#issuecomment-1315846775
<!---bot-comment-->
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](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* No users to tag found in teams: `tvmscript` <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a diff in pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13396:
URL: https://github.com/apache/tvm/pull/13396#discussion_r1023297485
##########
tests/python/unittest/test_tvmscript_roundtrip.py:
##########
@@ -3458,6 +3458,15 @@ def func() -> None:
return func
+def implicit_evaluate():
+ @T.prim_func
+ def func(A: T.Buffer[1, "int32"]):
+ T.evaluate(T.assume(A[0] == 5))
+ A[0] = 10
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
junrushao commented on code in PR #13396:
URL: https://github.com/apache/tvm/pull/13396#discussion_r1023328978
##########
tests/python/unittest/test_tvmscript_roundtrip.py:
##########
@@ -3458,6 +3458,15 @@ def func() -> None:
return func
+def implicit_evaluate():
+ @T.prim_func
+ def func(A: T.Buffer[1, "int32"]):
+ T.evaluate(T.assume(A[0] == 5))
+ A[0] = 10
Review Comment:
Ah I see. Thanks for the clarification!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
junrushao commented on PR #13396:
URL: https://github.com/apache/tvm/pull/13396#issuecomment-1315970348
Thanks for the great update! This helps a lot with the usability of TVMScript :-) Let's get it in ASAP!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a diff in pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13396:
URL: https://github.com/apache/tvm/pull/13396#discussion_r1023296204
##########
python/tvm/script/parser/tir/parser.py:
##########
@@ -411,6 +412,10 @@ def visit_expr_stmt(self: Parser, node: doc.Expr) -> None:
if isinstance(res, Frame):
res.add_callback(partial(res.__exit__, None, None, None))
res.__enter__()
+ elif isinstance(res, PrimExpr):
+ T.evaluate(res)
+ elif isinstance(res, (int, bool)):
+ T.evaluate(tvm.tir.const(res))
Review Comment:
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 an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
junrushao commented on code in PR #13396:
URL: https://github.com/apache/tvm/pull/13396#discussion_r1023332191
##########
python/tvm/script/parser/tir/parser.py:
##########
@@ -411,6 +412,10 @@ def visit_expr_stmt(self: Parser, node: doc.Expr) -> None:
if isinstance(res, Frame):
res.add_callback(partial(res.__exit__, None, None, None))
res.__enter__()
+ elif isinstance(res, PrimExpr):
+ T.evaluate(res)
+ elif isinstance(res, (int, bool)):
+ T.evaluate(tvm.tir.const(res))
Review Comment:
This is a good idea!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #13396:
URL: https://github.com/apache/tvm/pull/13396#issuecomment-1315851513
This PR is in response to CI failures found in https://github.com/apache/tvm/pull/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13396:
URL: https://github.com/apache/tvm/pull/13396#issuecomment-1315846779
<!---bot-comment-->
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](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* No users to tag found in teams: `tvmscript` <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao commented on a diff in pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
junrushao commented on code in PR #13396:
URL: https://github.com/apache/tvm/pull/13396#discussion_r1023260390
##########
src/printer/tvmscript_printer.cc:
##########
@@ -1275,16 +1275,11 @@ Doc TVMScriptPrinter::VisitStmt_(const SeqStmtNode* op) {
}
Doc TVMScriptPrinter::VisitStmt_(const EvaluateNode* op) {
- if (auto* call = op->value.as<CallNode>()) {
- if (call->op.same_as(builtin::assume())) {
- Doc doc;
- doc << tir_prefix_ << ".assume(" << Print(call->args[0]) << ")";
- return doc;
- }
- }
-
+ // When parsing TVMScript, a PrimExpr that occurs as a statement is
+ // automatically wrapped in `tir::Evaluate`. Therefore, when
+ // printing we only need to print the value.
Doc doc;
- doc << tir_prefix_ << ".evaluate(" << Print(op->value) << ")";
+ doc << Print(op->value);
Review Comment:
How about we only sugar `CallNode`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a diff in pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13396:
URL: https://github.com/apache/tvm/pull/13396#discussion_r1023299110
##########
src/printer/tvmscript_printer.cc:
##########
@@ -1275,16 +1275,11 @@ Doc TVMScriptPrinter::VisitStmt_(const SeqStmtNode* op) {
}
Doc TVMScriptPrinter::VisitStmt_(const EvaluateNode* op) {
- if (auto* call = op->value.as<CallNode>()) {
- if (call->op.same_as(builtin::assume())) {
- Doc doc;
- doc << tir_prefix_ << ".assume(" << Print(call->args[0]) << ")";
- return doc;
- }
- }
-
+ // When parsing TVMScript, a PrimExpr that occurs as a statement is
+ // automatically wrapped in `tir::Evaluate`. Therefore, when
+ // printing we only need to print the value.
Doc doc;
- doc << tir_prefix_ << ".evaluate(" << Print(op->value) << ")";
+ doc << Print(op->value);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #13396: [TVMScript] Use tir::Evaluate if expression is in statement context
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #13396:
URL: https://github.com/apache/tvm/pull/13396#issuecomment-1315920808
> 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++.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org