[Relax][Bugfix] Apply FuseOps to nested DataflowBlock #17033
Conversation
While it is ill-formed for control-flow to occur within a `DataflowBlock`, it is legal for a `DataflowBlock` to be contained within a control-flow. Prior to this commit, the `FuseOps` and `FuseOpsByPattern` transforms erroneously skipped `DataflowBlock` instances that were contained within a `relax::If` node. This commit updates `FuseOps` to apply operator fusion to any dataflow block, regardless of whether it is found at the top level of a a Relax function. Co-authored-by: Chris Sullivan <[email protected]>
|
This PR is identical to the now-closed PR #17022, as a CI restart prevented the CI from running on the original PR. |
| } else if (leaf_expr->IsInstance<ConstantNode>() || leaf_expr->IsInstance<ShapeExprNode>() || | ||
| leaf_expr->IsInstance<PrimValueNode>() || leaf_expr->IsInstance<StringImmNode>() || | ||
| leaf_expr->IsInstance<DataTypeImmNode>()) { | ||
| } else { |
There was a problem hiding this comment.
@tqchen Responding to the comment here, with conversation migrated to the new PR.
Is this intended change? Seems that we should consider sass where constant value(and there may not be a binding pt)
This change is intentional, and maintains the same behavior for constants as before. Prior to this change, the else if condition was entered for five of the six child classes of relax::LeafExprNode, and the else branch with the LOG(FATAL) was only entered for a relax::Var.
With this change, the behavior of all LeafExpr subclasses is the same: If encoutered without being previously assigned to a group, they are treated as a group of size one.
There was a problem hiding this comment.
This resolves an issue that results from the use of PostOrderVisit here, in VisitUnsupportedNode. It looks like this function is intended to collect all Var/Constant nodes that are part of the RHS of a binding (e.g. extracting A and B from C = R.add(A,B)). However, it doesn't collect any bindings that may be part of a nested expression. As a result, variable bindings that are part of a conditional's body would erroneously trigger the LOG(FATAL) in VisitLeaf.
While it is ill-formed for control-flow to occur within a
DataflowBlock, it is legal for aDataflowBlockto be contained within a control-flow. Prior to this commit, theFuseOpsandFuseOpsByPatterntransforms erroneously skippedDataflowBlockinstances that were contained within arelax::Ifnode.This commit updates
FuseOpsto apply operator fusion to any dataflow block, regardless of whether it is found at the top level of a a Relax function.