Lunderberg commented on code in PR #16594:
URL: https://github.com/apache/tvm/pull/16594#discussion_r1500755912
##########
src/relax/transform/canonicalize_bindings.cc:
##########
@@ -247,7 +247,8 @@ class BindingCanonicalizer : public ExprMutator {
}
Expr VisitExpr_(const VarNode* var) override {
- Var new_var = GetRef<Var>(var);
Review Comment:
Double-checking, I think the reason this line is no longer necessary is due
to the `ExprMutator::VisitExpr_(new_var.get());` at the end of the function.
It may have been necessary during some of the development steps, but is no
longer necessary. Thinking through the reasoning:
1. The `var_remap_` would apply if `VisitVarDef`
2. Canonicalization updates a variable in `VisitVarDef` if the
`plan_.replace_binding` is populated.
3. From (1) and (2), anything that would be populated in `var_remap_` would
also be populated in `plan_.replace_bindings`
4. Whenever we populate `plan_.replace_binding`, we also populate
`plan_.replace_usage`.
5. From (3) and (4), anything that would be replaced by `var_remap_` could
instead be replaced by `plan_.replace_usage`.
So, I think it isn't necessary to have two calls to
`ExprMutator::VisitExpr_(const Varnode*)` in the implementation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
