In the tile and fuse of the first extract use, we add support
for scenarios where the results of the tiled op have uses
that are dominated by the scf.for_all. Specifically, we replace
the scf.for_all with a new scf.for_all that has an additional
shared_out and add the appropriate parallel insert slice op.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp | ||
---|---|---|
370 | Is containingOp always a scf::ForallOp? If we are not we should check that the returned object is not null otherwise we should use cast instead of dynamic cast. | |
376 | same problem here | |
385 | This needs to be erased through the rewriter. | |
386 | I believe you can use takeRegion in the rewriter? | |
390 | nit: this comment is not very informative :) | |
399 | nit: you only need one OpBuilder::InsertionGuard per function | |
421 | Can you update the doc? | |
mlir/test/Dialect/Linalg/transform-op-fuse-into-containing.mlir | ||
310 | Is it expected that this linalg generic is still there? I would have expected that both destination would be replaced by the result of scf.forall? |
Thanks for the review. I will add some more tests and then upload a new PR.
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp | ||
---|---|---|
370 | I believe it is and most of the code assumes so. I will change the dyn_cast to cast. | |
376 | Here not sure whether producerOp is always a Linalg generic. So I will add a null check here and return early if it fails. | |
385 | Sure will change. | |
386 | Yes, I will change it to use takeBody which should make it much cleaner. Thanks! | |
390 | Agreed :) Will add a better comment there. | |
399 | Sure will remove this one. | |
421 | Will do. I will leave this as is and append more info on the second returned value. | |
mlir/test/Dialect/Linalg/transform-op-fuse-into-containing.mlir | ||
310 | Yes I believe so. The reason is that %0#1 is not being used inside the scf.forall and so we don't know how to slice it without making some assumptions. |
Address Thomas' comments and add 2 more tests.
One test shows both dominated and not dominated cases.
The second test shows cases where the producer op doesn't
have identity maps.
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp | ||
---|---|---|
362 | Should this be !containingOp->isAncestor(user) ? After this change, fuse_into_containing_op generates invalid IR for an IREE test case: A producer is fused into a scf.forall op, but it is has two users inside of the scf.forall. The generated code uses the result of the scf.forall inside of the body of the scf.forall. |
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp | ||
---|---|---|
362 | yes good catch! Thank you for your patch that fixes the issue. |
Should this be !containingOp->isAncestor(user) ?
After this change, fuse_into_containing_op generates invalid IR for an IREE test case: A producer is fused into a scf.forall op, but it is has two users inside of the scf.forall. The generated code uses the result of the scf.forall inside of the body of the scf.forall.