[mlir][linalg] Fix a DCE crash with memref<0x..> and the op has uses #73908
Conversation
|
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir Author: Kohei Yamaguchi (sott0n) ChangesThe DCE for LinalgOp when processing operands as Fixes #73547 Full diff: https://github.com/llvm/llvm-project/pull/73908.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
index 58af9995548e939..323ded3aadcd3a6 100644
--- a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
+++ b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
@@ -2101,7 +2101,7 @@ struct EraseDeadLinalgOp : public OpInterfaceRewritePattern<LinalgOp> {
auto mt = llvm::dyn_cast<MemRefType>(opOperand.get().getType());
if (!mt)
continue;
- if (llvm::is_contained(op.getShape(&opOperand), 0)) {
+ if (llvm::is_contained(op.getShape(&opOperand), 0) && op->use_empty()) {
rewriter.eraseOp(op);
return success();
}
diff --git a/mlir/test/Dialect/Linalg/canonicalize.mlir b/mlir/test/Dialect/Linalg/canonicalize.mlir
index e875bae4730946b..42188c01be16a57 100644
--- a/mlir/test/Dialect/Linalg/canonicalize.mlir
+++ b/mlir/test/Dialect/Linalg/canonicalize.mlir
@@ -28,7 +28,7 @@ func.func @memref_cast(%a: index, %b: index) -> memref<?x?xf32> {
}
func.func @dce_zero_memref(%arg0 : memref<0xf32>, %arg1: tensor<0xf32>) -> tensor<0xf32> {
- // memref<0x32> is expected to be dce'ed
+ // memref<0xf32> is expected to be dce'ed
memref.copy %arg0, %arg0 : memref<0xf32> to memref<0xf32>
// tensor<0xf32> cannot be dce'ed
@@ -47,6 +47,33 @@ func.func @dce_zero_memref(%arg0 : memref<0xf32>, %arg1: tensor<0xf32>) -> tenso
// -----
+#accesses = [
+ affine_map<(i) -> (i)>,
+ affine_map<(i) -> (i)>
+]
+
+#trait = {
+ indexing_maps = #accesses,
+ iterator_types = ["parallel"]
+}
+
+func.func @no_dce_zero_memref(%arg0 : memref<0xf32>, %arg1: tensor<0xf32>) -> tensor<0xf32> {
+ // memref<0xf32> cannot be dce'ed
+ %2 = linalg.generic #trait ins(%arg0: memref<0xf32>) outs(%arg1 : tensor<0xf32>) {
+ ^bb(%0: f32, %1: f32) :
+ linalg.yield %1 : f32
+ } -> tensor<0xf32>
+
+ return %2: tensor<0xf32>
+}
+
+// CHECK-LABEL: @no_dce_zero_memref
+// CHECK-SAME: %[[ARG0:[a-zA-Z0-9_]+]]: memref<0xf32>
+// CHECK-SAME: %[[ARG1:[a-zA-Z0-9_]+]]: tensor<0xf32>
+// CHECK-NEXT: linalg.generic
+
+// -----
+
func.func @dce_self_linalg_copy(%arg0 : memref<?xf32>) {
linalg.copy ins(%arg0: memref<?xf32>) outs(%arg0: memref<?xf32>)
return
|
|
@nicolasvasilache could you review this PR? (i can't add reviewers) |
| } | ||
|
|
||
| // CHECK-LABEL: @no_dce_zero_memref | ||
| // CHECK-SAME: %[[ARG0:[a-zA-Z0-9_]+]]: memref<0xf32> |
There was a problem hiding this comment.
We don't need to capture the variables with ARG0 and ARG1?
There was a problem hiding this comment.
Thanks for your comment! I removed it.
|
|
||
| func.func @no_dce_zero_memref(%arg0 : memref<0xf32>, %arg1: tensor<0xf32>) -> tensor<0xf32> { | ||
| // memref<0xf32> cannot be dce'ed | ||
| %2 = linalg.generic #trait ins(%arg0: memref<0xf32>) outs(%arg1 : tensor<0xf32>) { |
There was a problem hiding this comment.
hmm I thought we had tightened the op semantics to not have mixed memref/tensor anymore.
I would have expected a verification error here.
There was a problem hiding this comment.
Thank you for your review!
It seems to me that the linalg.GenericOp verification does not perform any meaningful action.
https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp#L1081
Do you think we should add a verify that prohibits the mix of memref/tensor at linalg.GenericOp's verification?
There was a problem hiding this comment.
I think that it would be a good idea to verify whether the mix of memref/tensor is present in the operand of inputs in the verifyStructuredOpInterface of LinalgOp.
However, I'm unsure whether it is correct to restrict the mix of memref/tensor in the input for the entire LinalgOp. Is it correct?
For example, in the linalg_effects test, there is a case where tensor/memref mixing exists in the inputs of linalg.matmul, and it is unclear whether such mixing is allowed in linalg.matmul.
There was a problem hiding this comment.
@nicolasvasilache Sorry for forgetting a mention... What do you think about it?
|
duplicated with #80660 |
The DCE for LinalgOp when processing operands as
memref<0x..>causes a crash if this op has uses. This PR addresses it.Fixes #73547