[Mlir-commits] [mlir] [mlir][SPIR-V] Share the memory operands mask verifier between both masks (PR #225520)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Wed Sep 23 04:05:48 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir-spirv
Author: Arseniy Obolenskiy (aobolensk)
<details>
<summary>Changes</summary>
Fix the CopyMemory printer along the way, it echoed a lone mask as a source mask, which reparsed as target only mask and failed verification.
---
Full diff: https://github.com/llvm/llvm-project/pull/225520.diff
2 Files Affected:
- (modified) mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp (+60-106)
- (modified) mlir/test/Dialect/SPIRV/IR/memory-ops.mlir (+22-1)
``````````diff
diff --git a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
index 0b98825774c80..e4586913cfe24 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;
}
@@ -172,70 +172,68 @@ static LogicalResult verifyLoadStorePtrAndValTypes(LoadStoreOpTy op, Value ptr,
return success();
}
-template <typename MemoryOpTy>
-static LogicalResult verifyMemoryAccessAttribute(MemoryOpTy memoryOp) {
+namespace {
+/// Whether the pointer a memory operands mask applies to is read through,
+/// written through, or both.
+enum class MemoryAccessKind { Read, Write, ReadWrite };
+} // namespace
+
+/// Verifies the memory operands mask `memAccessAttr` of `op` and its companion
+/// alignment attribute `alignmentAttr`. `kind` tells how the pointer the mask
+/// applies to is accessed.
+static LogicalResult
+verifyMemoryAccessAttribute(Operation *op,
+ spirv::MemoryAccessAttr memAccessAttr,
+ Attribute alignmentAttr, MemoryAccessKind kind) {
// ODS checks for attributes values. Just need to verify that if the
// memory-access attribute is Aligned, then the alignment attribute must be
// present.
- spirv::MemoryAccessAttr memAccessAttr = memoryOp.getMemoryAccessAttr();
if (!memAccessAttr) {
// Alignment attribute shouldn't be present if memory access attribute is
// not present.
- if (memoryOp.getAlignmentAttr()) {
- return memoryOp.emitOpError(
+ if (alignmentAttr) {
+ return op->emitOpError(
"invalid alignment specification without aligned memory access "
"specification");
}
return success();
}
- spirv::MemoryAccessAttr memAccess = memAccessAttr;
+ spirv::MemoryAccess memAccess = memAccessAttr.getValue();
- if (!memAccess) {
- return memoryOp.emitOpError("invalid memory access specifier: ")
- << memAccessAttr;
- }
-
- // MakePointerAvailable applies to writes through the pointer, so it is
- // invalid for Load (which only reads through it).
- if constexpr (std::is_same_v<MemoryOpTy, LoadOp>) {
- if (spirv::bitEnumContainsAll(memAccess.getValue(),
- spirv::MemoryAccess::MakePointerAvailable)) {
- return memoryOp.emitOpError(
- "not compatible with memory operand 'MakePointerAvailable'");
- }
+ // MakePointerAvailable applies to writes through the pointer.
+ if (kind == MemoryAccessKind::Read &&
+ spirv::bitEnumContainsAll(memAccess,
+ spirv::MemoryAccess::MakePointerAvailable)) {
+ return op->emitOpError(
+ "not compatible with memory operand 'MakePointerAvailable'");
}
- // 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.
- if constexpr (std::is_same_v<MemoryOpTy, StoreOp> ||
- std::is_same_v<MemoryOpTy, CopyMemoryOp>) {
- if (spirv::bitEnumContainsAll(memAccess.getValue(),
- spirv::MemoryAccess::MakePointerVisible)) {
- return memoryOp.emitOpError(
- "not compatible with memory operand 'MakePointerVisible'");
- }
+ // MakePointerVisible applies to reads through the pointer.
+ if (kind == MemoryAccessKind::Write &&
+ spirv::bitEnumContainsAll(memAccess,
+ spirv::MemoryAccess::MakePointerVisible)) {
+ return op->emitOpError(
+ "not compatible with memory operand 'MakePointerVisible'");
}
- if (spirv::bitEnumContainsAny(memAccess.getValue(),
+ if (spirv::bitEnumContainsAny(memAccess,
spirv::MemoryAccess::MakePointerAvailable |
spirv::MemoryAccess::MakePointerVisible) &&
- !spirv::bitEnumContainsAll(memAccess.getValue(),
+ !spirv::bitEnumContainsAll(memAccess,
spirv::MemoryAccess::NonPrivatePointer)) {
- return memoryOp.emitOpError(
+ return op->emitOpError(
"memory operand 'MakePointerAvailable' or 'MakePointerVisible' "
"requires 'NonPrivatePointer' to also be specified");
}
- if (spirv::bitEnumContainsAll(memAccess.getValue(),
- spirv::MemoryAccess::Aligned)) {
- if (!memoryOp.getAlignmentAttr()) {
- return memoryOp.emitOpError("missing alignment value");
+ if (spirv::bitEnumContainsAll(memAccess, spirv::MemoryAccess::Aligned)) {
+ if (!alignmentAttr) {
+ return op->emitOpError("missing alignment value");
}
} else {
- if (memoryOp.getAlignmentAttr()) {
- return memoryOp.emitOpError(
+ if (alignmentAttr) {
+ return op->emitOpError(
"invalid alignment specification with non-aligned memory access "
"specification");
}
@@ -243,64 +241,13 @@ static LogicalResult verifyMemoryAccessAttribute(MemoryOpTy memoryOp) {
return success();
}
-// TODO Make sure to merge this and the previous function into one template
-// parameterized by memory access attribute name and alignment. Doing so now
-// results in VS2017 in producing an internal error (at the call site) that's
-// not detailed enough to understand what is happening.
+/// Verifies the default (non-Source) memory operands mask of `memoryOp`.
template <typename MemoryOpTy>
-static LogicalResult verifySourceMemoryAccessAttribute(MemoryOpTy memoryOp) {
- // ODS checks for attributes values. Just need to verify that if the
- // memory-access attribute is Aligned, then the alignment attribute must be
- // present.
- spirv::MemoryAccessAttr memAccessAttr = memoryOp.getSourceMemoryAccessAttr();
- if (!memAccessAttr) {
- // Alignment attribute shouldn't be present if memory access attribute is
- // not present.
- if (memoryOp.getSourceAlignmentAttr()) {
- return memoryOp.emitOpError(
- "invalid alignment specification without aligned memory access "
- "specification");
- }
- return success();
- }
-
- spirv::MemoryAccessAttr memAccess = memAccessAttr;
-
- if (!memAccess) {
- return memoryOp.emitOpError("invalid memory access specifier: ")
- << memAccess;
- }
-
- // The source mask applies to the read through the source pointer, so it
- // cannot include MakePointerAvailable, which applies to writes.
- if (spirv::bitEnumContainsAll(memAccess.getValue(),
- spirv::MemoryAccess::MakePointerAvailable)) {
- return memoryOp.emitOpError(
- "not compatible with memory operand 'MakePointerAvailable'");
- }
-
- if (spirv::bitEnumContainsAll(memAccess.getValue(),
- spirv::MemoryAccess::MakePointerVisible) &&
- !spirv::bitEnumContainsAll(memAccess.getValue(),
- spirv::MemoryAccess::NonPrivatePointer)) {
- return memoryOp.emitOpError(
- "memory operand 'MakePointerAvailable' or 'MakePointerVisible' "
- "requires 'NonPrivatePointer' to also be specified");
- }
-
- if (spirv::bitEnumContainsAll(memAccess.getValue(),
- spirv::MemoryAccess::Aligned)) {
- if (!memoryOp.getSourceAlignmentAttr()) {
- return memoryOp.emitOpError("missing alignment value");
- }
- } else {
- if (memoryOp.getSourceAlignmentAttr()) {
- return memoryOp.emitOpError(
- "invalid alignment specification with non-aligned memory access "
- "specification");
- }
- }
- return success();
+static LogicalResult verifyMemoryAccessAttribute(MemoryOpTy memoryOp,
+ MemoryAccessKind kind) {
+ return verifyMemoryAccessAttribute(memoryOp.getOperation(),
+ memoryOp.getMemoryAccessAttr(),
+ memoryOp.getAlignmentAttr(), kind);
}
//===----------------------------------------------------------------------===//
@@ -459,7 +406,7 @@ LogicalResult LoadOp::verify() {
if (failed(verifyLoadStorePtrAndValTypes(*this, getPtr(), getValue()))) {
return failure();
}
- return verifyMemoryAccessAttribute(*this);
+ return verifyMemoryAccessAttribute(*this, MemoryAccessKind::Read);
}
//===----------------------------------------------------------------------===//
@@ -505,7 +452,7 @@ LogicalResult StoreOp::verify() {
// OpTypePointer whose Type operand is the same as the type of Object."
if (failed(verifyLoadStorePtrAndValTypes(*this, getPtr(), getValue())))
return failure();
- return verifyMemoryAccessAttribute(*this);
+ return verifyMemoryAccessAttribute(*this, MemoryAccessKind::Write);
}
//===----------------------------------------------------------------------===//
@@ -588,10 +535,17 @@ LogicalResult CopyMemoryOp::verify() {
if (targetType != sourceType)
return emitOpError("both operands must be pointers to the same type");
- if (failed(verifyMemoryAccessAttribute(*this)))
+ // A lone mask applies to both operands. Only the first of two masks is
+ // Target-only.
+ MemoryAccessKind targetKind = getSourceMemoryAccess()
+ ? MemoryAccessKind::Write
+ : MemoryAccessKind::ReadWrite;
+ if (failed(verifyMemoryAccessAttribute(*this, targetKind)))
return failure();
- return verifySourceMemoryAccessAttribute(*this);
+ return verifyMemoryAccessAttribute(
+ getOperation(), getSourceMemoryAccessAttr(), getSourceAlignmentAttr(),
+ MemoryAccessKind::Read);
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir b/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
index 78dabf67ece35..19c0a1c2a7518 100644
--- a/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
@@ -746,7 +746,17 @@ 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
+}
+
+// -----
+
+func.func @copy_memory_target_make_pointer_available_missing_non_private() {
+ %0 = spirv.Variable : !spirv.ptr<f32, Function>
+ %1 = spirv.Variable : !spirv.ptr<f32, Function>
+ // expected-error @+1 {{op memory operand 'MakePointerAvailable' or 'MakePointerVisible' requires 'NonPrivatePointer' to also be specified}}
+ "spirv.CopyMemory"(%0, %1) {memory_access=#spirv.memory_access<MakePointerAvailable>, source_memory_access=#spirv.memory_access<None>} : (!spirv.ptr<f32, Function>, !spirv.ptr<f32, Function>) -> ()
spirv.Return
}
@@ -788,6 +798,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" %{{[0-9]+}}, "Function" %{{[0-9]+}} ["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" %{{[0-9]+}}, "Function" %{{[0-9]+}} ["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
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/225520
More information about the Mlir-commits
mailing list