[Mlir-commits] [mlir] [mlir][SPIR-V] Enforce physical storage buffer pointer decorations on GlobalVariable (PR #203600)

[llvmlistbot at llvm.org]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-spirv

Author: Arseniy Obolenskiy (aobolensk)

<details>
<summary>Changes</summary>

Enable the rule that was already enforced by spirv.Variable (in case of SPV_KHR_physical_storage_buffer ext) requiring exactly one of AliasedPointer/RestrictPointer

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


4 Files Affected:

- (modified) mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp (+3-28) 
- (modified) mlir/lib/Dialect/SPIRV/IR/SPIRVOpUtils.h (+7) 
- (modified) mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp (+45) 
- (modified) mlir/test/Dialect/SPIRV/IR/structure-ops.mlir (+50) 


``````````diff
diff --git a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
index a49bcff441b9d..f9c03bf3b88c0 100644
--- a/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/MemoryOps.cpp
@@ -670,34 +670,9 @@ LogicalResult VariableOp::verify() {
              << "' attribute (only allowed in spirv.GlobalVariable)";
   }
 
-  // From SPV_KHR_physical_storage_buffer:
-  // > If an OpVariable's pointee type is a pointer (or array of pointers) in
-  // > PhysicalStorageBuffer storage class, then the variable must be decorated
-  // > with exactly one of AliasedPointer or RestrictPointer.
-  auto pointeePtrType = dyn_cast<spirv::PointerType>(getPointeeType());
-  if (!pointeePtrType) {
-    if (auto pointeeArrayType = dyn_cast<spirv::ArrayType>(getPointeeType())) {
-      pointeePtrType =
-          dyn_cast<spirv::PointerType>(pointeeArrayType.getElementType());
-    }
-  }
-
-  if (pointeePtrType && pointeePtrType.getStorageClass() ==
-                            spirv::StorageClass::PhysicalStorageBuffer) {
-    bool hasAliasedPtr =
-        getDecorationAttr(spirv::Decoration::AliasedPointer) != nullptr;
-    bool hasRestrictPtr =
-        getDecorationAttr(spirv::Decoration::RestrictPointer) != nullptr;
-
-    if (!hasAliasedPtr && !hasRestrictPtr)
-      return emitOpError() << " with physical buffer pointer must be decorated "
-                              "either 'AliasedPointer' or 'RestrictPointer'";
-
-    if (hasAliasedPtr && hasRestrictPtr)
-      return emitOpError()
-             << " with physical buffer pointer must have exactly one "
-                "aliasing decoration";
-  }
+  if (failed(verifyPhysicalStorageBufferDecorations(getOperation(),
+                                                    getPointeeType())))
+    return failure();
 
   return success();
 }
diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOpUtils.h b/mlir/lib/Dialect/SPIRV/IR/SPIRVOpUtils.h
index e60fd53737d52..d084fbcb8b3ae 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOpUtils.h
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOpUtils.h
@@ -37,4 +37,11 @@ LogicalResult extractValueFromConstOp(Operation *op, int32_t &value);
 LogicalResult verifyMemorySemantics(Operation *op,
                                     spirv::MemorySemantics memorySemantics);
 
+/// Verifies the SPV_KHR_physical_storage_buffer rule that a variable whose
+/// pointee is a pointer (or array of pointers) in the PhysicalStorageBuffer
+/// storage class must be decorated with exactly one of AliasedPointer or
+/// RestrictPointer.
+LogicalResult verifyPhysicalStorageBufferDecorations(Operation *op,
+                                                     Type pointeeType);
+
 } // namespace mlir::spirv
diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
index 119cdcf0fe4fa..4cb6e6bbf6579 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVOps.cpp
@@ -90,6 +90,46 @@ spirv::verifyMemorySemantics(Operation *op,
   return success();
 }
 
+LogicalResult spirv::verifyPhysicalStorageBufferDecorations(Operation *op,
+                                                            Type pointeeType) {
+  // From SPV_KHR_physical_storage_buffer:
+  // > If an OpVariable's pointee type is a pointer (or array of pointers) in
+  // > PhysicalStorageBuffer storage class, then the variable must be decorated
+  // > with exactly one of AliasedPointer or RestrictPointer.
+  auto pointeePtrType = dyn_cast<spirv::PointerType>(pointeeType);
+  if (!pointeePtrType) {
+    if (auto pointeeArrayType = dyn_cast<spirv::ArrayType>(pointeeType)) {
+      pointeePtrType =
+          dyn_cast<spirv::PointerType>(pointeeArrayType.getElementType());
+    }
+  }
+
+  if (!pointeePtrType || pointeePtrType.getStorageClass() !=
+                             spirv::StorageClass::PhysicalStorageBuffer)
+    return success();
+
+  auto getDecorationAttr = [op](spirv::Decoration decoration) {
+    return op->getAttr(spirv::getDecorationString(decoration));
+  };
+
+  bool hasAliasedPtr =
+      getDecorationAttr(spirv::Decoration::AliasedPointer) != nullptr;
+  bool hasRestrictPtr =
+      getDecorationAttr(spirv::Decoration::RestrictPointer) != nullptr;
+
+  if (!hasAliasedPtr && !hasRestrictPtr)
+    return op->emitOpError()
+           << " with physical buffer pointer must be decorated "
+              "either 'AliasedPointer' or 'RestrictPointer'";
+
+  if (hasAliasedPtr && hasRestrictPtr)
+    return op->emitOpError()
+           << " with physical buffer pointer must have exactly one "
+              "aliasing decoration";
+
+  return success();
+}
+
 void spirv::printVariableDecorations(Operation *op, OpAsmPrinter &printer,
                                      SmallVectorImpl<StringRef> &elidedAttrs) {
   // Print optional descriptor binding
@@ -1390,6 +1430,11 @@ LogicalResult spirv::GlobalVariableOp::verify() {
     }
   }
 
+  Type pointeeType = cast<spirv::PointerType>(getType()).getPointeeType();
+  if (failed(
+          verifyPhysicalStorageBufferDecorations(getOperation(), pointeeType)))
+    return failure();
+
   return success();
 }
 
diff --git a/mlir/test/Dialect/SPIRV/IR/structure-ops.mlir b/mlir/test/Dialect/SPIRV/IR/structure-ops.mlir
index 858408de1cacd..c0b73b926c764 100644
--- a/mlir/test/Dialect/SPIRV/IR/structure-ops.mlir
+++ b/mlir/test/Dialect/SPIRV/IR/structure-ops.mlir
@@ -640,6 +640,56 @@ spirv.module Logical GLSL450 {
 
 // -----
 
+spirv.module Logical GLSL450 {
+  spirv.GlobalVariable @var0 {aliased_pointer} :
+    !spirv.ptr<!spirv.ptr<f32, PhysicalStorageBuffer>, Private>
+  spirv.GlobalVariable @var1 {restrict_pointer} :
+    !spirv.ptr<!spirv.ptr<f32, PhysicalStorageBuffer>, Private>
+}
+
+// -----
+
+spirv.module Logical GLSL450 {
+  // expected-error @+1 {{must be decorated either 'AliasedPointer' or 'RestrictPointer'}}
+  spirv.GlobalVariable @var0 :
+    !spirv.ptr<!spirv.ptr<f32, PhysicalStorageBuffer>, Private>
+}
+
+// -----
+
+spirv.module Logical GLSL450 {
+  // expected-error @+1 {{must have exactly one aliasing decoration}}
+  spirv.GlobalVariable @var0 {aliased_pointer, restrict_pointer} :
+    !spirv.ptr<!spirv.ptr<f32, PhysicalStorageBuffer>, Private>
+}
+
+// -----
+
+spirv.module Logical GLSL450 {
+  spirv.GlobalVariable @var0 {aliased_pointer} :
+    !spirv.ptr<!spirv.array<4x!spirv.ptr<f32, PhysicalStorageBuffer>>, Private>
+  spirv.GlobalVariable @var1 {restrict_pointer} :
+    !spirv.ptr<!spirv.array<4x!spirv.ptr<f32, PhysicalStorageBuffer>>, Private>
+}
+
+// -----
+
+spirv.module Logical GLSL450 {
+  // expected-error @+1 {{must be decorated either 'AliasedPointer' or 'RestrictPointer'}}
+  spirv.GlobalVariable @var0 :
+    !spirv.ptr<!spirv.array<4x!spirv.ptr<f32, PhysicalStorageBuffer>>, Private>
+}
+
+// -----
+
+spirv.module Logical GLSL450 {
+  // expected-error @+1 {{must have exactly one aliasing decoration}}
+  spirv.GlobalVariable @var0 {aliased_pointer, restrict_pointer} :
+    !spirv.ptr<!spirv.array<4x!spirv.ptr<f32, PhysicalStorageBuffer>>, Private>
+}
+
+// -----
+
 //===----------------------------------------------------------------------===//
 // spirv.module
 //===----------------------------------------------------------------------===//

``````````

</details>


https://github.com/llvm/llvm-project/pull/203600