Lunderberg commented on PR #16732: URL: https://github.com/apache/tvm/pull/16732#issuecomment-2029645251
This is an interesting one. 1. The `DFPatternMatcher` will unwrap variables when attempting to find a match ([link](https://github.com/apache/tvm/blob/main/src/relax/ir/dataflow_matcher.cc#L62)), as part of `ExtractMatchedExpr`. 2. The `ExprPatternMatcher` will save the value of each variable binding is saved in order to ([link](https://github.com/apache/tvm/blob/main/src/relax/ir/dataflow_matcher.cc#L1152)). 3. The `ExprPatternMatcher` checks for a match in the override for `Expr VisitExpr(const Expr&)`. So the call to `rewriter` with the assert fails is made when `ExprPatternMatcher` is checking the variable usage in the body of the `SeqExpr`. This gets unwrapped by `DFPatternMatcher`, and identified as an expression whose value is equal to the match itself. This potential bug existed all the way back to https://github.com/apache/tvm/pull/15578. However, until https://github.com/apache/tvm/pull/16732, the `relax::Var` body of a `SeqExpr` would never be the first match encountered. Any time a `relax::Var` would have matched, the expression to which it was bound would have matched first, hiding the later match. I think the resolution is to update condition (1). Where currently, we unwrap all variable bindings as part of a match, we should instead unwrap all variable bindings except for the top-level of the match. This could be most easily implemented by having the `ExprPatternMatcher::VisitExpr` skip the call to `TryRewrite` for `relax::Var`. I'll try this out and see if it resolves the issue. -- 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
