Details
- Reviewers
mravishankar nicolasvasilache gysit
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I am in favor of this approach! In its current form it may lead to additional pad tensor operations when padding groups of Linalg operations. The reason is that makeComposedPadHighOp can only remove redundant pad tensor operations if the operations are padded in the correct order from producer to consumer. Up to now this is guaranteed since padding a consumer will only work once the extract slice of the padded producer is available. This implicit control needs to replaced by an explicit control of the transformation order. I would thus suggest to improve control over code transformations before landing the patch. Or we should at least be sure it does not affect performance of the current benchmarks.
| mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp | ||
|---|---|---|
| 185 | You can avoid the loop as well here by using the OpResult result number: while (auto linalgOp = opOperand->get().getDefiningOp<LinalgOp>()) { OpResult result = opOperand->get().cast<OpResult>(); opOperand = &linalgOp.getOutputOperand(result.getResultNumber()); } | |
| mlir/test/Dialect/Linalg/pad.mlir | ||
| 516 | This test shows a interesting side-effect of the change :). We see a pad here since the matmul is apparently padded after the generic. Without the the change the generic is padded after the matmul since only once padding applied to matmul there is an extract slice which enables padding the generic... I actually believe this test and also the test pad_multiple may now fail depending on the pattern application order... I would thus suggest to adapt TestLinalgCodegenStrategy.cpp (https://github.com/llvm/llvm-project/blob/35dfa78ff8d44733b1c805309f0bbd4a8c960897/mlir/test/lib/Dialect/Linalg/TestLinalgCodegenStrategy.cpp#L203) to pad only operations with a specific name. It may be possible to do this depending the fusion flag or we may want to control it with an additional command line flag (e.g., pad-anchor-op-only). Then it should be possible to write a really simple test that contains a fill and a matmul or matvec where only the matmul / matvec is padded. | |
You can avoid the loop as well here by using the OpResult result number:
while (auto linalgOp = opOperand->get().getDefiningOp<LinalgOp>()) {
}