[MLIR][MemRef] Add verifier check for index count vs memref rank in generic_atomic_rmw#189229
[MLIR][MemRef] Add verifier check for index count vs memref rank in generic_atomic_rmw#189229joker-eph wants to merge 1 commit into
Conversation
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-memref Author: Mehdi Amini (joker-eph) Changes
Fixes #178211 Assisted-by: Claude Code Full diff: https://github.com/llvm/llvm-project/pull/189229.diff 2 Files Affected:
diff --git a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
index 404b2aacf1450..8728e66919baf 100644
--- a/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp
@@ -1593,6 +1593,12 @@ LogicalResult GenericAtomicRMWOp::verify() {
if (getResult().getType() != body.getArgument(0).getType())
return emitOpError("expected block argument of the same type result type");
+ auto memrefType = cast<MemRefType>(getMemref().getType());
+ if (getIndices().size() != static_cast<size_t>(memrefType.getRank()))
+ return emitOpError("index count (")
+ << getIndices().size() << ") does not match memref rank ("
+ << memrefType.getRank() << ")";
+
bool hasSideEffects =
body.walk([&](Operation *nestedOp) {
if (isMemoryEffectFree(nestedOp))
diff --git a/mlir/test/Dialect/MemRef/invalid.mlir b/mlir/test/Dialect/MemRef/invalid.mlir
index af068d8ca8e95..eeb1a80278f1a 100644
--- a/mlir/test/Dialect/MemRef/invalid.mlir
+++ b/mlir/test/Dialect/MemRef/invalid.mlir
@@ -1125,6 +1125,17 @@ func.func @atomic_yield_type_mismatch(%I: memref<10xf32>, %i : index) {
// -----
+func.func @generic_atomic_rmw_rank_mismatch(%arg0: memref<i32>, %idx: index) {
+ // expected-error@+1 {{index count (1) does not match memref rank (0)}}
+ %r = memref.generic_atomic_rmw %arg0[%idx] : memref<i32> {
+ ^bb0(%v: i32):
+ memref.atomic_yield %v : i32
+ }
+ return
+}
+
+// -----
+
#map0 = affine_map<(d0) -> (d0 floordiv 8, d0 mod 8)>
func.func @memref_realloc_layout(%src : memref<256xf32, #map0>) -> memref<?xf32>{
// expected-error@+1 {{unsupported layout}}
|
| @@ -1593,6 +1593,12 @@ LogicalResult GenericAtomicRMWOp::verify() { | |||
| if (getResult().getType() != body.getArgument(0).getType()) | |||
| return emitOpError("expected block argument of the same type result type"); | |||
|
|
|||
| auto memrefType = cast<MemRefType>(getMemref().getType()); | |||
| if (getIndices().size() != static_cast<size_t>(memrefType.getRank())) | |||
There was a problem hiding this comment.
Didn't double-check, let's ensure the op is already constrained to only accept ranked memrefs.
There was a problem hiding this comment.
It is, and it reminded me I can write as:
MemRefType memrefType = getMemref().getType();
Instead of
auto memrefType = cast<MemRefType>(getMemref().getType());
Because getMemref() returns a TypedValue.
ecbf644 to
55c7ddd
Compare
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) MLIRMLIR.Dialect/MemRef/invalid.mlirIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
🪟 Windows x64 Test Results
Failed Tests(click on a test name to see its output) MLIRMLIR.Dialect/MemRef/invalid.mlirIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
…eneric_atomic_rmw `memref.generic_atomic_rmw` did not verify that the number of index operands matches the rank of the memref. This caused a crash during lowering to LLVM when an index count mismatch was present (e.g., a rank-0 memref accessed with one index). Add a verifier check to `GenericAtomicRMWOp::verify()` that emits a proper error diagnostic instead of crashing in the lowering pattern. Fixes llvm#178211 Assisted-by: Claude Code
|
Can't reproduce the failure locally, rebased and pushed just to see... |
55c7ddd to
ddfc72c
Compare
|
I didn't fetch before rebase, I reproduce the failure now. This is because of b813b0b which changed this to use IndexedMemoryAccessOpInterface which already catch this case in the verifier now. So this change is obsolete and I can close the bug as fixed. |
Pull request was closed
memref.generic_atomic_rmwdid not verify that the number of index operands matches the rank of the memref. This caused a crash during lowering to LLVM when an index count mismatch was present (e.g., a rank-0 memref accessed with one index).Fixes #178211
Assisted-by: Claude Code