[MLIR] [Transforms] Let lowerToLoopsUsingSCFForOp delete target op, fixes #83252 #83256
Conversation
The function mlir::scf::lowerToLoopsUsingSCFForOp was not deleting its (structured) target op, resulting in IR with the expected loop nest in front of the still remaining (structured) op, e.g. a linalg.matmul.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir Author: None (lhunloh) ChangesThe function mlir::scf::lowerToLoopsUsingSCFForOp was not deleting its (structured) target op, resulting in IR with the expected loop nest in front of the still remaining (structured) op, e.g. a linalg.matmul. This would fix issue #83252 Full diff: https://github.com/llvm/llvm-project/pull/83256.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
index 1a84a59ddb69df..e1e9be858b251e 100644
--- a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
+++ b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
@@ -1133,5 +1133,6 @@ mlir::scf::lowerToLoopsUsingSCFForOp(RewriterBase &rewriter,
if (failed(op.generateScalarImplementation(rewriter, op.getLoc(), ivs))) {
return failure();
}
+ rewriter.eraseOp(op);
return loops;
}
|
|
@llvm/pr-subscribers-mlir-scf Author: None (lhunloh) ChangesThe function mlir::scf::lowerToLoopsUsingSCFForOp was not deleting its (structured) target op, resulting in IR with the expected loop nest in front of the still remaining (structured) op, e.g. a linalg.matmul. This would fix issue #83252 Full diff: https://github.com/llvm/llvm-project/pull/83256.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
index 1a84a59ddb69df..e1e9be858b251e 100644
--- a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
+++ b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp
@@ -1133,5 +1133,6 @@ mlir::scf::lowerToLoopsUsingSCFForOp(RewriterBase &rewriter,
if (failed(op.generateScalarImplementation(rewriter, op.getLoc(), ivs))) {
return failure();
}
+ rewriter.eraseOp(op);
return loops;
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Oops, another commit sneaked its way into this PR, resulting in formatter issues. I reverted that one, so only commit 2f1ef50 remains, as originally intended. |
The caller of function `lowerToLoopsUsingSCFForOp`, `ConvertToLoopsOp` was not deleting its (structured) target op, resulting in IR with the expected loop nest in front of the still remaining (structured) op, e.g. a linalg.matmul.
|
You are right, that's a better place, since methods around |
|
Can you add a test case please? |
This tests that the op is indeed deleted by transform.structured.convert_to_loops.
|
Test added. And it indeed fails when rewriter.eraseOp is commented out. |
|
#83537 implemented the same changes a day later, so closing this... |
The function mlir::scf::lowerToLoopsUsingSCFForOp was not deleting its (structured) target op, resulting in IR with the expected loop nest in front of the still remaining (structured) op, e.g. a linalg.matmul.
This would fix issue #83252