Add logical objectFifo hoisting pass #679
Conversation
jtuyls
requested review from
MaheshRavishankar,
nirvedhmeshram,
yzhang93 and
Abhishek-Varma
as code owners
| @@ -213,6 +213,11 @@ def AMDAIEHoistForLoopAffineApply : Pass<"iree-amdaie-hoist-for-affine-apply"> { | |||
| let constructor = "mlir::iree_compiler::AMDAIE::createAMDAIEHoistForLoopAffineApplyPass()"; | |||
| } | |||
|
|
|||
| def AMDAIEHoistLogicalObjFifo : Pass<"iree-amdaie-hoist-logical-objectfifo"> { | |||
| let summary = "Hoist logical objectFifo operations to the scope of its operands."; | |||
There was a problem hiding this comment.
| let summary = "Hoist logical objectFifo operations to the scope of its operands."; | |
| let summary = "Hoist logical objectFifo operations to the scope of a most nested operand, without hoisting through workgroup, controlcode, or func ops."; |
There was a problem hiding this comment.
I guess it makes sense to not hoist through these ops because they're isolated from above (could you check that instead?)
There was a problem hiding this comment.
Scrap that idea, they're not isolated from above
| AMDAIE::LogicalObjectFifoFromMemrefOp op) { | ||
| Operation *parentOp = op; | ||
| while (parentOp) { | ||
| Operation *newParentOp = parentOp->getParentOp(); |
There was a problem hiding this comment.
I interpret this as
"If the new parent is a proper ancestor of any operand, keep hoisting"
which seems like not what you want?
There was a problem hiding this comment.
"If the new parent is a proper ancestor of any operand, keep hoisting"
No, then it breaks,
| Operation *parentOp = getOperation(); | ||
| IRRewriter rewriter(parentOp->getContext()); | ||
| parentOp->walk([&](AMDAIE::LogicalObjectFifoFromMemrefOp op) { | ||
| (void)hoistLogicalObjFifoOp(rewriter, op); |
There was a problem hiding this comment.
I'm not sure if walk enumerates all the ops it will visit before hand, I don't think so, so I think you actually visit all the hoisted ops twice (once after hoisting) here.
FWIW I find code that
- walk to find targets
- process targets
easier to grok.
There was a problem hiding this comment.
so I think you actually visit all the hoisted ops twice (once after hoisting)
The hoisted ops will be inserted before the ancestor op, so should not be visited again?
jtuyls
force-pushed
the
hoist-logical-obj-fifo
branch
from
907b52a to
1e82b7d
Compare
jtuyls
force-pushed
the
hoist-logical-obj-fifo
branch
from
1e82b7d to
53e7b1e
Compare
|
@newling Could you help check the PR again? |
| Operation *parentOp = getOperation(); | ||
| IRRewriter rewriter(parentOp->getContext()); | ||
| parentOp->walk([&](AMDAIE::LogicalObjectFifoFromMemrefOp op) { | ||
| (void)hoistLogicalObjFifoOp(rewriter, op); |
Follow-up on #676, which blocks subsumption when operands are in the same scope. Hoists
amdaie.logicalobjectfifo.from_memrefops as much as possible, which enables more subsumption opportunities.