You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "Lunderberg (via GitHub)" <gi...@apache.org> on 2024/03/25 16:34:01 UTC
[PR] [Relax] Improve CanonicalizeBindings in DataflowVar edge case [tvm]
Lunderberg opened a new pull request, #16783:
URL: https://github.com/apache/tvm/pull/16783
If there is a trivial binding of `Var = DataflowVar`, but the non-dataflow variable is never used outside the dataflow block in which is is declared, then we should keep the name of the upstream `DataflowVar`, as it is more likely to be the human-readable name (e.g. a function parameter).
--
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
Re: [PR] [Relax] Improve CanonicalizeBindings in DataflowVar edge case [tvm]
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #16783:
URL: https://github.com/apache/tvm/pull/16783#discussion_r1538290478
##########
src/relax/transform/canonicalize_bindings.cc:
##########
@@ -91,18 +91,20 @@ class CanonicalizePlanner : public ExprVisitor {
bound_to = opt.value();
}
- if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>()) {
+ if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>() ||
+ !visitor.used_outside_home_dataflow_.count(bound_var)) {
// Case 1: Var = Var
// Case 2: DataflowVar = Var
// Case 3: DataflowVar = DataflowVar
+ // Case 4a: Var = DataflowVar, but used outside this DataflowBlock
//
// For these three cases, the trivial binding can be
Review Comment:
```suggestion
// For these four cases, the trivial binding can be
```
:wink:
##########
src/relax/transform/canonicalize_bindings.cc:
##########
@@ -91,18 +91,20 @@ class CanonicalizePlanner : public ExprVisitor {
bound_to = opt.value();
}
- if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>()) {
+ if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>() ||
+ !visitor.used_outside_home_dataflow_.count(bound_var)) {
// Case 1: Var = Var
// Case 2: DataflowVar = Var
// Case 3: DataflowVar = DataflowVar
+ // Case 4a: Var = DataflowVar, but used outside this DataflowBlock
//
// For these three cases, the trivial binding can be
Review Comment:
```suggestion
// For these four cases, the trivial binding can be
```
:wink:
--
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
Re: [PR] [Relax] Improve CanonicalizeBindings in DataflowVar edge case [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #16783:
URL: https://github.com/apache/tvm/pull/16783#discussion_r1539278031
##########
src/relax/transform/canonicalize_bindings.cc:
##########
@@ -91,18 +91,20 @@ class CanonicalizePlanner : public ExprVisitor {
bound_to = opt.value();
}
- if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>()) {
+ if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>() ||
+ !visitor.used_outside_home_dataflow_.count(bound_var)) {
// Case 1: Var = Var
// Case 2: DataflowVar = Var
// Case 3: DataflowVar = DataflowVar
+ // Case 4a: Var = DataflowVar, but used outside this DataflowBlock
//
// For these three cases, the trivial binding can be
Review Comment:
Off by one errors, my ~~two~~ one nemesis! (Thank you, and fixed.)
--
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
Re: [PR] [Relax] Improve CanonicalizeBindings in DataflowVar edge case [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on PR #16783:
URL: https://github.com/apache/tvm/pull/16783#issuecomment-2021040212
@tvm-bot rerun
--
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
Re: [PR] [Relax] Improve CanonicalizeBindings in DataflowVar edge case [tvm]
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #16783:
URL: https://github.com/apache/tvm/pull/16783#discussion_r1538290737
##########
src/relax/transform/canonicalize_bindings.cc:
##########
@@ -91,18 +91,20 @@ class CanonicalizePlanner : public ExprVisitor {
bound_to = opt.value();
}
- if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>()) {
+ if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>() ||
+ !visitor.used_outside_home_dataflow_.count(bound_var)) {
// Case 1: Var = Var
// Case 2: DataflowVar = Var
// Case 3: DataflowVar = DataflowVar
+ // Case 4a: Var = DataflowVar, but used outside this DataflowBlock
//
// For these three cases, the trivial binding can be
Review Comment:
```suggestion
// For these four cases, the trivial binding can be
```
:wink:
##########
src/relax/transform/canonicalize_bindings.cc:
##########
@@ -91,18 +91,20 @@ class CanonicalizePlanner : public ExprVisitor {
bound_to = opt.value();
}
- if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>()) {
+ if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>() ||
+ !visitor.used_outside_home_dataflow_.count(bound_var)) {
// Case 1: Var = Var
// Case 2: DataflowVar = Var
// Case 3: DataflowVar = DataflowVar
+ // Case 4a: Var = DataflowVar, but used outside this DataflowBlock
Review Comment:
```suggestion
// Case 4a: Var = DataflowVar, but not used outside this DataflowBlock
```
It appears the text was copied from case 4b. I also think it would be clearer to write out that it is the var that is not used outside the DF block
--
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
Re: [PR] [Relax] Improve CanonicalizeBindings in DataflowVar edge case [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg commented on code in PR #16783:
URL: https://github.com/apache/tvm/pull/16783#discussion_r1539285615
##########
src/relax/transform/canonicalize_bindings.cc:
##########
@@ -91,18 +91,20 @@ class CanonicalizePlanner : public ExprVisitor {
bound_to = opt.value();
}
- if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>()) {
+ if (bound_var.as<DataflowVarNode>() || !bound_to.as<DataflowVarNode>() ||
+ !visitor.used_outside_home_dataflow_.count(bound_var)) {
// Case 1: Var = Var
// Case 2: DataflowVar = Var
// Case 3: DataflowVar = DataflowVar
+ // Case 4a: Var = DataflowVar, but used outside this DataflowBlock
Review Comment:
Thank you, and updated the comment to be more explicit. I've also changed "this DataflowBlock" to "the DataflowBlock containing the binding", since this function is called after the entire function is visited, not during the visit of any specific dataflow block.
--
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
Re: [PR] [Relax] Improve CanonicalizeBindings in DataflowVar edge case [tvm]
Posted by "Lunderberg (via GitHub)" <gi...@apache.org>.
Lunderberg merged PR #16783:
URL: https://github.com/apache/tvm/pull/16783
--
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