Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1991942344 > In principle, I'm in favor of always allowing a user to omit the var and giving a warning rather than an error if they do it for a statement with a non-void return. We can consider that as a later change if others are in favor. I agree, and would like to add this in a later PR. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg merged PR #16641: URL: https://github.com/apache/tvm/pull/16641 -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1987301530 All CI tests passing, ready for review/merge. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1985926454 Rebased onto main, as the CI failures seemed unrelated to this PR, and `main` has had some CI fixes recently. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1971078553 > We could even have the normalizer do it. I like this as a long-term solution. I've updated this PR to only change the TVMScript parsing, and not the printing, along with disabling the related unit tests. That way, they can be re-enabled in a follow-up PR. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
slyubomirsky commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1970076815 Ah I see, we don't really want the pretty printer to have to be checking if a var is later used. We could take the policy of always inlining a unit tuple value (or always doing it in the pretty printer), as this brings no harm. We could even have the normalizer do it. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1967668483 True. I'd see the mismatch as the degree to which relax should be compatible with the python environment. For lisp, I'd expect `nil` to be the empty tuple, but for Python, I'd expect `None` to be distinct.from the empty tuple. >I'm not sure what our stance would be on parser roundtripping: I'm fine with saying that is permitted to omit the binding and that we should go with one way or the other for parser roundtripping. Unfortunately, this runs a little bit deeper. Since a size-zero tuple may be used at a later point in the relax function, omitting the binding requires inspecting the function to see if that binding is being used. Even with something like `dummy_var: R.Tuple() = R.print(...)`, the `R.Tuple()` struct info is insufficient to know if `dummy_var` is used later in the function. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
slyubomirsky commented on code in PR #16641:
URL: https://github.com/apache/tvm/pull/16641#discussion_r1504846413
##
python/tvm/script/parser/relax/parser.py:
##
@@ -274,7 +274,21 @@ def post_visit_local_function(self: Parser, node:
doc.Expr) -> None:
@dispatch.register(token="relax", type_name="Expr")
def visit_expr_stmt(self: Parser, node: doc.Expr) -> None:
value = self.eval_expr(node.value)
-if value is not None:
+if isinstance(value, relax.Expr):
+var = R.emit(value)
+IRBuilder.name("_", var)
+is_void_value = (
+isinstance(var.struct_info, relax.TupleStructInfo) and
len(var.struct_info.fields) == 0
+)
+
+if not is_void_value:
+self.report_error(
+node,
+f"Non-void relax expressions must be bound to a variable, "
+f"but expression of type {var.struct_info} was used as a
statement.",
+)
Review Comment:
Those are good points. For the first case, we could have a warning (as C and
other languages do with the right settings) for ignoring a return value. The
second one is an interesting issue. I think it suggests that expecting an exact
textual match for the parser roundtripping is too strict of a criterion for
this situation, since a "statement" can always be written as `_ = ...` and it
would be a choice as to whether to write it that way or use the friendlier
syntax. It would make it harder to write automatic tests, true. For a
systematic solution, maybe we could formalize the idea of a desugaring step for
testing purposes?
--
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-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
slyubomirsky commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1967446140 Well, I wouldn't call that's not a type system ambiguity, but it could become one in terms of parser roundtripping. It's just that an empty tuple (unit value) cannot contain anything and is immutable, so its use is historically like a null value in functional languages. We do have the odd quirk that we have both the `null_value` operator that returns a void value and also the unit value. Personally, I think it would be better to use a unit value in all cases since there would be no reason to have an empty tuple at all. I'm not sure what our stance would be on parser roundtripping: I'm fine with saying that is _permitted_ to omit the binding and that we should go with one way or the other for parser roundtripping. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1965751186 And after poking at it, it looks like there an ambiguity in the relax type system. In most cases, a zero-field `TupleStructInfo` is used to represent a void type ([example](https://github.com/apache/tvm/blob/main/include/tvm/relax/struct_info.h#L457)). However, in some cases, a zero-field `TupleStructInfo` is used to represent, well, a zero-field tuple ([example](https://github.com/apache/tvm/blob/main/tests/python/relax/test_tvmscript_parser.py#L1369)). Eliding the variable binding for an actual void type make sense, as there are no valid uses of a void type. However, a zero-field tuple can be treated as an object, and so removing its variable binding may result in undefined usage. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on PR #16641: URL: https://github.com/apache/tvm/pull/16641#issuecomment-1965741548 > Does this intersect with the quirky parsing for if-else? For example, if the value returned in an if-else is of void type. Would it be safe not to write out the return var? Would it still roundtrip? Good call on a test to add. Looks like this change does cause an issue with round-trips when the elided binding is the last binding in an if/else block. I've added a currently-failing unit test for the round-trip. -- 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-unsubscr...@tvm.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on code in PR #16641:
URL: https://github.com/apache/tvm/pull/16641#discussion_r1503572139
##
python/tvm/script/parser/relax/parser.py:
##
@@ -274,7 +274,21 @@ def post_visit_local_function(self: Parser, node:
doc.Expr) -> None:
@dispatch.register(token="relax", type_name="Expr")
def visit_expr_stmt(self: Parser, node: doc.Expr) -> None:
value = self.eval_expr(node.value)
-if value is not None:
+if isinstance(value, relax.Expr):
+var = R.emit(value)
+IRBuilder.name("_", var)
+is_void_value = (
+isinstance(var.struct_info, relax.TupleStructInfo) and
len(var.struct_info.fields) == 0
+)
+
+if not is_void_value:
+self.report_error(
+node,
+f"Non-void relax expressions must be bound to a variable, "
+f"but expression of type {var.struct_info} was used as a
statement.",
+)
Review Comment:
At the moment, because I wanted to make the minimal change that would
support common cases. I think it would be good to remove the restriction
altogether, but for the first step, I wanted to make the restriction be
explicit.
There's a couple of concerns I could see with allowing non-void return value
to be implicitly ignored.
* Prevent accidentally unused values. If `cls.add1(a,b)` does an in-place
update of `a`, but `cls.add2(a,b)` returns a new value, using `cls.add2(a,b)`
without assigning to a value would likely be an error.
* Round-trip TVMScript -> Relax -> TVMScript without a pre-processing pass.
Checking if a value has void type can done while printing the IR. Checking
whether a non-void variable could be omitted would require a pre-processing
step to find any downstream users.
I don't think either of those are definitive arguments, but I figured I'd
handle the unambiguous beneficial cases first, with a follow-up PR to relax the
restriction.
--
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-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
Lunderberg commented on code in PR #16641:
URL: https://github.com/apache/tvm/pull/16641#discussion_r1503572139
##
python/tvm/script/parser/relax/parser.py:
##
@@ -274,7 +274,21 @@ def post_visit_local_function(self: Parser, node:
doc.Expr) -> None:
@dispatch.register(token="relax", type_name="Expr")
def visit_expr_stmt(self: Parser, node: doc.Expr) -> None:
value = self.eval_expr(node.value)
-if value is not None:
+if isinstance(value, relax.Expr):
+var = R.emit(value)
+IRBuilder.name("_", var)
+is_void_value = (
+isinstance(var.struct_info, relax.TupleStructInfo) and
len(var.struct_info.fields) == 0
+)
+
+if not is_void_value:
+self.report_error(
+node,
+f"Non-void relax expressions must be bound to a variable, "
+f"but expression of type {var.struct_info} was used as a
statement.",
+)
Review Comment:
At the moment, because I wanted to make the minimal change that would
support common cases. I think it would be good to remove the restriction
altogether, but for the first step, I wanted to make the restriction be
explicit.
There's a couple of concerns I could see with allowing non-void return value
to be implicitly ignored.
* Prevent accidentally unused values. If two IRModule instances An in-place
operator that performs `a = a + b` may be represented as `cls.add(a, b)`. This
woul
* Round-trip TVMScript -> Relax -> TVMScript without a pre-processing pass.
Checking if a value has void type can done while printing the IR. Checking
whether a non-void variable could be omitted would require a pre-processing
step to find any downstream users.
I don't think either of those are definitive arguments, but I figured I'd
handle the unambiguous beneficial cases first, with a follow-up PR to relax the
restriction.
--
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-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [TVMScript] Allow use of relax.Expr with void type as a statement [tvm]
slyubomirsky commented on code in PR #16641:
URL: https://github.com/apache/tvm/pull/16641#discussion_r1503451237
##
python/tvm/script/parser/relax/parser.py:
##
@@ -274,7 +274,21 @@ def post_visit_local_function(self: Parser, node:
doc.Expr) -> None:
@dispatch.register(token="relax", type_name="Expr")
def visit_expr_stmt(self: Parser, node: doc.Expr) -> None:
value = self.eval_expr(node.value)
-if value is not None:
+if isinstance(value, relax.Expr):
+var = R.emit(value)
+IRBuilder.name("_", var)
+is_void_value = (
+isinstance(var.struct_info, relax.TupleStructInfo) and
len(var.struct_info.fields) == 0
+)
+
+if not is_void_value:
+self.report_error(
+node,
+f"Non-void relax expressions must be bound to a variable, "
+f"but expression of type {var.struct_info} was used as a
statement.",
+)
Review Comment:
I wonder if we should even have this as a rule. Why not let users evaluate
expressions without binding them regardless of their return type?
--
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-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
