[Mlir-commits] [mlir] [mlir][SPIR-V] Fix CopyMemory handling of a lone memory operands mask (PR #226969)

[Arseniy Obolenskiy]

https://github.com/aobolensk updated https://github.com/llvm/llvm-project/pull/226969

>From 53ec042f09d1717f898def5587484f3e6fa6403c Mon Sep 17 00:00:00 2001
From: Arseniy Obolenskiy <arseniy.obolenskiy at amd.com>
Date: Mon, 28 Sep 2026 14:24:22 +0200
Subject: [PATCH 1/2] [mlir][SPIR-V] Fix CopyMemory handling of a lone memory
 operands mask

---
 mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp    | 24 ++++++++++++++--------
 mlir/test/Dialect/SPIRV/IR/memory-ops.mlir | 13 +++++++++++-
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
index 0b98825774c806..95608355cd0ee3 100644
--- a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
@@ -109,19 +109,19 @@ static void printSourceMemoryAccessAttribute(
     std::optional<spirv::MemoryAccess> memoryAccessAtrrValue = std::nullopt,
     std::optional<uint32_t> alignmentAttrValue = std::nullopt) {
 
-  printer << ", ";
-
   // Print optional memory access attribute.
-  if (auto memAccess = (memoryAccessAtrrValue ? memoryAccessAtrrValue
-                                              : memoryOp.getMemoryAccess())) {
+  if (auto memAccess =
+          (memoryAccessAtrrValue ? memoryAccessAtrrValue
+                                 : memoryOp.getSourceMemoryAccess())) {
     elidedAttrs.push_back(memoryOp.getSourceMemoryAccessAttrName());
 
-    printer << " [\"" << stringifyMemoryAccess(*memAccess) << "\"";
+    printer << ", [\"" << stringifyMemoryAccess(*memAccess) << "\"";
 
     if (spirv::bitEnumContainsAll(*memAccess, spirv::MemoryAccess::Aligned)) {
       // Print integer alignment attribute.
-      if (auto alignment = (alignmentAttrValue ? alignmentAttrValue
-                                               : memoryOp.getAlignment())) {
+      if (auto alignment =
+              (alignmentAttrValue ? alignmentAttrValue
+                                  : memoryOp.getSourceAlignment())) {
         elidedAttrs.push_back(memoryOp.getSourceAlignmentAttrName());
         printer << ", " << *alignment;
       }
@@ -208,10 +208,16 @@ static LogicalResult verifyMemoryAccessAttribute(MemoryOpTy memoryOp) {
 
   // MakePointerVisible applies to reads through the pointer, so it is invalid
   // for Store and for the Target operand of CopyMemory, both of which only
-  // write through it.
+  // write through it. A CopyMemory mask is Target-only when a Source mask
+  // follows it, otherwise it applies to both operands.
   if constexpr (std::is_same_v<MemoryOpTy, StoreOp> ||
                 std::is_same_v<MemoryOpTy, CopyMemoryOp>) {
-    if (spirv::bitEnumContainsAll(memAccess.getValue(),
+    bool writeOnly = true;
+    if constexpr (std::is_same_v<MemoryOpTy, CopyMemoryOp>)
+      writeOnly = memoryOp.getSourceMemoryAccess().has_value();
+
+    if (writeOnly &&
+        spirv::bitEnumContainsAll(memAccess.getValue(),
                                   spirv::MemoryAccess::MakePointerVisible)) {
       return memoryOp.emitOpError(
           "not compatible with memory operand 'MakePointerVisible'");
diff --git a/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir b/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
index 78dabf67ece35b..2f823198b804a8 100644
--- a/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
@@ -746,7 +746,7 @@ func.func @copy_memory_target_bad_operand() {
   %0 = spirv.Variable : !spirv.ptr<f32, Function>
   %1 = spirv.Variable : !spirv.ptr<f32, Function>
   // expected-error @+1 {{op not compatible with memory operand 'MakePointerVisible'}}
-  "spirv.CopyMemory"(%0, %1) {memory_access=#spirv.memory_access<MakePointerVisible|NonPrivatePointer>} : (!spirv.ptr<f32, Function>, !spirv.ptr<f32, Function>) -> ()
+  "spirv.CopyMemory"(%0, %1) {memory_access=#spirv.memory_access<MakePointerVisible|NonPrivatePointer>, source_memory_access=#spirv.memory_access<None>} : (!spirv.ptr<f32, Function>, !spirv.ptr<f32, Function>) -> ()
   spirv.Return
 }
 
@@ -788,6 +788,17 @@ func.func @copy_memory_print_maa() {
   // CHECK: spirv.CopyMemory "Function" %{{.*}}, "Function" %{{.*}} ["Aligned", 4], ["Aligned", 8] : f32
   "spirv.CopyMemory"(%0, %1) {source_memory_access=#spirv.memory_access<Aligned>, memory_access=#spirv.memory_access<Aligned>, source_alignment=8 : i32, alignment=4 : i32} : (!spirv.ptr<f32, Function>, !spirv.ptr<f32, Function>) -> ()
 
+  // A lone mask applies to both operands, so it may carry MakePointerVisible,
+  // MakePointerAvailable, or both. It must stay a lone mask when printed.
+  // CHECK: spirv.CopyMemory "Function" %{{.*}}, "Function" %{{.*}} ["MakePointerVisible|NonPrivatePointer"] : f32
+  "spirv.CopyMemory"(%0, %1) {memory_access=#spirv.memory_access<MakePointerVisible|NonPrivatePointer>} : (!spirv.ptr<f32, Function>, !spirv.ptr<f32, Function>) -> ()
+
+  // CHECK: spirv.CopyMemory "Function" %{{.*}}, "Function" %{{.*}} ["MakePointerAvailable|MakePointerVisible|NonPrivatePointer"] : f32
+  "spirv.CopyMemory"(%0, %1) {memory_access=#spirv.memory_access<MakePointerAvailable|MakePointerVisible|NonPrivatePointer>} : (!spirv.ptr<f32, Function>, !spirv.ptr<f32, Function>) -> ()
+
+  // CHECK: spirv.CopyMemory "Function" %{{.*}}, "Function" %{{.*}} ["MakePointerAvailable|NonPrivatePointer"], ["MakePointerVisible|NonPrivatePointer"] : f32
+  "spirv.CopyMemory"(%0, %1) {memory_access=#spirv.memory_access<MakePointerAvailable|NonPrivatePointer>, source_memory_access=#spirv.memory_access<MakePointerVisible|NonPrivatePointer>} : (!spirv.ptr<f32, Function>, !spirv.ptr<f32, Function>) -> ()
+
   spirv.Return
 }
 

>From 7d04e19fad45bf20d3d9f317a0cb907e284aa8c5 Mon Sep 17 00:00:00 2001
From: Arseniy Obolenskiy <arseniy.obolenskiy at amd.com>
Date: Wed, 30 Sep 2026 18:55:21 +0200
Subject: [PATCH 2/2] sourceOnly

---
 mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
index 95608355cd0ee3..debce94cd98cd0 100644
--- a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
@@ -212,11 +212,11 @@ static LogicalResult verifyMemoryAccessAttribute(MemoryOpTy memoryOp) {
   // follows it, otherwise it applies to both operands.
   if constexpr (std::is_same_v<MemoryOpTy, StoreOp> ||
                 std::is_same_v<MemoryOpTy, CopyMemoryOp>) {
-    bool writeOnly = true;
+    bool sourceOnly = true;
     if constexpr (std::is_same_v<MemoryOpTy, CopyMemoryOp>)
-      writeOnly = memoryOp.getSourceMemoryAccess().has_value();
+      sourceOnly = memoryOp.getSourceMemoryAccess().has_value();
 
-    if (writeOnly &&
+    if (sourceOnly &&
         spirv::bitEnumContainsAll(memAccess.getValue(),
                                   spirv::MemoryAccess::MakePointerVisible)) {
       return memoryOp.emitOpError(