[TVMScript] Allow use of relax.Expr with void type as a statement #16641
Conversation
Lunderberg
force-pushed
the
tvmscript_allow_void_relax_expr_as_stmt
branch
from
bb4252b to
3ca3c2d
Compare
| 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.", | ||
| ) |
There was a problem hiding this 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?
There was a problem hiding this 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 ofa, butcls.add2(a,b)returns a new value, usingcls.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.
There was a problem hiding this 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?
slyubomirsky
left a comment
slyubomirsky
left a comment
There was a problem hiding this comment.
Thank you for making this PR, definitely a great quality-of-life addition. I think there's a test case we should check for this: 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?
Lunderberg
force-pushed
the
tvmscript_allow_void_relax_expr_as_stmt
branch
from
c2b0602 to
09090ec
Compare
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. |
|
And after poking at it, it looks like there an ambiguity in the relax type system. In most cases, a zero-field |
|
Well, I wouldn't call that's not a type system ambiguity, but it would be an ambiguity in 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 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. |
|
True. I'd see the mismatch as the degree to which relax should be compatible with the python environment. For lisp, I'd expect
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 |
|
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. |
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 a follow-up commit to apache#16641. While parsing of relax expressions without a variable binding could be implemented at that point (e.g. `R.assert_op(condition)` instead of `dummy_var = R.assert_op(condition)`), the corresponding printing changes could not. This was because a variable that satisfies `relax::HasVoidStructInfo(var)` could still be used later in the function, and removing its binding would result in use of an undefined variable. This commit normalizes use of void-type variables to an in-line `R.tuple()`. This simplifies the relax function, and also allows the binding of void-type variables to be hidden.
Lunderberg
force-pushed
the
tvmscript_allow_void_relax_expr_as_stmt
branch
from
d7b27c6 to
69c200b
Compare
This is a follow-up commit to apache#16641. While parsing of relax expressions without a variable binding could be implemented at that point (e.g. `R.assert_op(condition)` instead of `dummy_var = R.assert_op(condition)`), the corresponding printing changes could not. This was because a variable that satisfies `relax::HasVoidStructInfo(var)` could still be used later in the function, and removing its binding would result in use of an undefined variable. This commit normalizes use of void-type variables to an in-line `R.tuple()`. This simplifies the relax function, and also allows the binding of void-type variables to be hidden.
Prior to this commit, TVMScript required all relax expressions to be part of an explicit assignment or return statement. While this matches the structure of the C++ IR types, this can be unexpected for functions that have no return value. For example, needing to assign the result of `R.print(...)` to a variable. This commit updates the TVMScript parser/printer to allow relax expressions to be used as statements, if they have a void return type. This allows use of `R.print(...)` and `R.assert_op(...)` to be called without assigning the result to an unused variable.
Lunderberg
force-pushed
the
tvmscript_allow_void_relax_expr_as_stmt
branch
from
69c200b to
8ed29a0
Compare
|
Rebased onto main, as the CI failures seemed unrelated to this PR, and |
|
All CI tests passing, ready for review/merge. |
There was a problem hiding this comment.
Thanks for responding to feedback. This will be a nice bit of syntactic sugar and along with #16658 will be a substantial improvement.
Edit: 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 a follow-up commit to apache#16641. While parsing of relax expressions without a variable binding could be implemented at that point (e.g. `R.assert_op(condition)` instead of `dummy_var = R.assert_op(condition)`), the corresponding printing changes could not. This was because a variable that satisfies `relax::HasVoidStructInfo(var)` could still be used later in the function, and removing its binding would result in use of an undefined variable. This commit normalizes use of void-type variables to an in-line `R.tuple()`. This simplifies the relax function, and also allows the binding of void-type variables to be hidden.
* [Relax] Normalize use of void-type variable to inline R.tuple() This is a follow-up commit to #16641. While parsing of relax expressions without a variable binding could be implemented at that point (e.g. `R.assert_op(condition)` instead of `dummy_var = R.assert_op(condition)`), the corresponding printing changes could not. This was because a variable that satisfies `relax::HasVoidStructInfo(var)` could still be used later in the function, and removing its binding would result in use of an undefined variable. This commit normalizes use of void-type variables to an in-line `R.tuple()`. This simplifies the relax function, and also allows the binding of void-type variables to be hidden. * Fix breakage in unit tests
…ache#16641) Prior to this commit, TVMScript required all relax expressions to be part of an explicit assignment or return statement. While this matches the structure of the C++ IR types, this can be unexpected for functions that have no return value. For example, needing to assign the result of `R.print(...)` to a variable. This commit updates the TVMScript parser/printer to allow relax expressions to be used as statements, if they have a void return type. This allows use of `R.print(...)` and `R.assert_op(...)` to be called without assigning the result to an unused variable.
…he#16658) * [Relax] Normalize use of void-type variable to inline R.tuple() This is a follow-up commit to apache#16641. While parsing of relax expressions without a variable binding could be implemented at that point (e.g. `R.assert_op(condition)` instead of `dummy_var = R.assert_op(condition)`), the corresponding printing changes could not. This was because a variable that satisfies `relax::HasVoidStructInfo(var)` could still be used later in the function, and removing its binding would result in use of an undefined variable. This commit normalizes use of void-type variables to an in-line `R.tuple()`. This simplifies the relax function, and also allows the binding of void-type variables to be hidden. * Fix breakage in unit tests
2 participants
Prior to this commit, TVMScript required all relax expressions to be part of an explicit assignment or return statement. While this matches the structure of the C++ IR types, this can be unexpected for functions that have no return value. For example, needing to assign the result of
R.print(...)to a variable.This commit updates the TVMScript parser/printer to allow relax expressions to be used as statements, if they have a void return type. This allows use of
R.print(...)andR.assert_op(...)to be called without assigning the result to an unused variable.