[MLIR] Add missing memory read effect on memref.reshape

[bondhugula]

The memory read effect on a memref.reshape argument was missing. This in turn led to
analyses relying on memory effects making incorrect conclusions.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Uday Bondhugula (bondhugula)

Changes

Add missing memory read effect on memref.reshape. This in turn leads to
analyses relying on memory effects making incorrect conclusions.

---

Full diff: https://github.com/llvm/llvm-project/pull/117130.diff

1 Files Affected:

• (modified) mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td (+2-1)

diff --git a/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td b/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
index c50df6ccd9aa56..a0d8d34f38237a 100644
--- a/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
+++ b/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
@@ -1529,7 +1529,8 @@ def MemRef_ReshapeOp: MemRef_Op<"reshape", [
   }];
 
   let arguments = (ins AnyRankedOrUnrankedMemRef:$source,
-                       MemRefRankOf<[AnySignlessInteger, Index], [1]>:$shape);
+                       Arg<MemRefRankOf<[AnySignlessInteger, Index], [1]>,
+                       "dynamically-sized shape", [MemRead]>:$shape);
   let results = (outs AnyRankedOrUnrankedMemRef:$result);
 
   let builders = [OpBuilder<

[llvmbot]

@llvm/pr-subscribers-mlir-memref

Author: Uday Bondhugula (bondhugula)

Changes

Add missing memory read effect on memref.reshape. This in turn leads to
analyses relying on memory effects making incorrect conclusions.

---

Full diff: https://github.com/llvm/llvm-project/pull/117130.diff

1 Files Affected:

• (modified) mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td (+2-1)

diff --git a/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td b/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
index c50df6ccd9aa56..a0d8d34f38237a 100644
--- a/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
+++ b/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
@@ -1529,7 +1529,8 @@ def MemRef_ReshapeOp: MemRef_Op<"reshape", [
   }];
 
   let arguments = (ins AnyRankedOrUnrankedMemRef:$source,
-                       MemRefRankOf<[AnySignlessInteger, Index], [1]>:$shape);
+                       Arg<MemRefRankOf<[AnySignlessInteger, Index], [1]>,
+                       "dynamically-sized shape", [MemRead]>:$shape);
   let results = (outs AnyRankedOrUnrankedMemRef:$result);
 
   let builders = [OpBuilder<

[cxy-1993]

Thanks for fixing this.

[joker-eph]

That should be testable I think? We don't have a transformation where this can be visible?

[bondhugula]

That should be testable I think? We don't have a transformation where this can be visible?

Yes, this should be testable. There are multiple affine analysis utilities that check for ops with mem read effects:

lib/Dialect/Affine/Transforms/AffineLoopInvariantCodeMotion.cpp:          (hasEffect<MemoryEffects::Read>(user, memref) &&
lib/Dialect/Affine/Utils/Utils.cpp:mlir::affine::hasNoInterveningEffect<mlir::MemoryEffects::Read,
lib/Dialect/Affine/Utils/Utils.cpp:    if (!affine::hasNoInterveningEffect<MemoryEffects::Read>(writeA, writeB,

Any of the passes relying on these utilities (affine-scalrep and affine-licm) will be able to exercise this. I can construct a test case.

affine.store ..., %M[...]  // S1.
memref.reshape %x, %M
affine.store ..., %M[...]  // S2.

Now, S1 should not be dead if the read effect is properly defined on memref.reshape.

[ftynse]

I suppose the intent of the question was to say that we should have a test to avoid regressing this behavior.

[joker-eph]

I suppose the intent of the question was to say that we should have a test to avoid regressing this behavior.

Yes: @bondhugula can you actually add a test for this change?