[Mlir-commits] [mlir] 326789c - [mlir][SPIR-V] Share the memory operands mask verifier between both masks (#225520)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Thu Oct 1 03:33:45 PDT 2026
Author: Arseniy Obolenskiy
Date: 2026-10-01T12:33:34+02:00
New Revision: 326789c0a3686fa47466ae92e9c9013073799673
URL: https://github.com/llvm/llvm-project/commit/326789c0a3686fa47466ae92e9c9013073799673
DIFF: https://github.com/llvm/llvm-project/commit/326789c0a3686fa47466ae92e9c9013073799673.diff
LOG: [mlir][SPIR-V] Share the memory operands mask verifier between both masks (#225520)
Verify the `CopyMemory` Target and Source masks through one shared
function instead of a separate Source path
Originally discussed in https://github.com/llvm/llvm-project/pull/213621
Added:
Modified:
mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
Removed:
################################################################################
diff --git a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
index debce94cd98cd..c99b6e04ef1ac 100644
--- a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
@@ -172,76 +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.
+ if (kind == MemoryAccessKind::Read &&
+ spirv::bitEnumContainsAll(memAccess,
+ spirv::MemoryAccess::MakePointerAvailable)) {
+ return op->emitOpError(
+ "not compatible with memory operand 'MakePointerAvailable'");
}
- // 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'");
- }
+ // 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'");
}
- // 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. 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>) {
- bool sourceOnly = true;
- if constexpr (std::is_same_v<MemoryOpTy, CopyMemoryOp>)
- sourceOnly = memoryOp.getSourceMemoryAccess().has_value();
-
- if (sourceOnly &&
- spirv::bitEnumContainsAll(memAccess.getValue(),
- spirv::MemoryAccess::MakePointerVisible)) {
- return memoryOp.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");
}
@@ -249,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);
}
//===----------------------------------------------------------------------===//
@@ -465,7 +406,7 @@ LogicalResult LoadOp::verify() {
if (failed(verifyLoadStorePtrAndValTypes(*this, getPtr(), getValue()))) {
return failure();
}
- return verifyMemoryAccessAttribute(*this);
+ return verifyMemoryAccessAttribute(*this, MemoryAccessKind::Read);
}
//===----------------------------------------------------------------------===//
@@ -511,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);
}
//===----------------------------------------------------------------------===//
@@ -594,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 2f823198b804a..b86c769b0f870 100644
--- a/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/memory-ops.mlir
@@ -752,6 +752,16 @@ func.func @copy_memory_target_bad_operand() {
// -----
+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
+}
+
+// -----
+
func.func @copy_memory_source_bad_operand() {
%0 = spirv.Variable : !spirv.ptr<f32, Function>
%1 = spirv.Variable : !spirv.ptr<f32, Function>
More information about the Mlir-commits
mailing list