address review comments

llvm · Nov 4, 2023 · a1ee18f · a1ee18f
1 parent de69c84
commit a1ee18f
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 7 deletions.
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td
@@ -56,10 +56,11 @@ def LLVM_AnyPointer : Type<CPred<"::llvm::isa<::mlir::LLVM::LLVMPointerType>($_s
                           "LLVM pointer type", "::mlir::LLVM::LLVMPointerType">;
 
 // Opaque pointer in a given address space.
-class LLVM_OpaquePointerInAddressSpace<int addressSpace> : Type<
-        CPred<
-          "::llvm::cast<::mlir::LLVM::LLVMPointerType>($_self).getAddressSpace() == "
-          # addressSpace>,
+class LLVM_PointerInAddressSpace<int addressSpace> : Type<
+  And<[LLVM_AnyPointer.predicate,
+      CPred<
+        "::llvm::cast<::mlir::LLVM::LLVMPointerType>($_self).getAddressSpace() == "
+        # addressSpace>]>,
   "Opaque LLVM pointer in address space " # addressSpace,
   "::mlir::LLVM::LLVMPointerType"> {
   let builderCall = "$_builder.getType<::mlir::LLVM::LLVMPointerType>("

diff --git a/mlir/include/mlir/Dialect/LLVMIR/NVVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/NVVMOps.td
@@ -19,8 +19,8 @@ include "mlir/Dialect/LLVMIR/LLVMOpBase.td"
 include "mlir/Interfaces/SideEffectInterfaces.td"
 include "mlir/Dialect/LLVMIR/BasicPtxBuilderInterface.td"
 
-def LLVM_ptr_global : LLVM_OpaquePointerInAddressSpace<1>;
-def LLVM_ptr_shared : LLVM_OpaquePointerInAddressSpace<3>;
+def LLVM_ptr_global : LLVM_PointerInAddressSpace<1>;
+def LLVM_ptr_shared : LLVM_PointerInAddressSpace<3>;
 
 //===----------------------------------------------------------------------===//
 // NVVM dialect definitions

diff --git a/mlir/include/mlir/Dialect/LLVMIR/ROCDLOps.td b/mlir/include/mlir/Dialect/LLVMIR/ROCDLOps.td
@@ -275,7 +275,7 @@ def ROCDL_wmma_i32_16x16x16_iu4 : ROCDL_Wmma_IntrOp<"wmma.i32.16x16x16.iu4">;
 // raw buffer mode).
 //===---------------------------------------------------------------------===//
 
-def ROCDLBufferRsrc : LLVM_OpaquePointerInAddressSpace<8>;
+def ROCDLBufferRsrc : LLVM_PointerInAddressSpace<8>;
 
 def ROCDL_MakeBufferRsrcOp :
   ROCDL_IntrOp<"make.buffer.rsrc", [], [0], [Pure], 1>,

diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -1954,6 +1954,13 @@ static bool isZeroAttribute(Attribute value) {
 }
 
 LogicalResult GlobalOp::verify() {
+  bool validType = isCompatibleOuterType(getType())
+                       ? !llvm::isa<LLVMVoidType, LLVMTokenType,
+                                    LLVMMetadataType, LLVMLabelType>(getType())
+                       : llvm::isa<PointerElementTypeInterface>(getType());
+  if (!validType)
+    return emitOpError(
+        "expects type to be a valid element type for an LLVM global");
   if ((*this)->getParentOp() && !satisfiesLLVMModule((*this)->getParentOp()))
     return emitOpError("must appear at the module level");
 

diff --git a/mlir/test/Dialect/LLVMIR/global.mlir b/mlir/test/Dialect/LLVMIR/global.mlir
@@ -117,6 +117,10 @@ llvm.mlir.global internal protected unnamed_addr @protected(42 : i32) : i32
 
 // -----
 
+// expected-error @+1 {{expects type to be a valid element type for an LLVM global}}
+llvm.mlir.global internal constant @constant(37.0) : !llvm.label
+
+// -----
 // expected-error @+1 {{'addr_space' failed to satisfy constraint: 32-bit signless integer attribute whose value is non-negative}}
 "llvm.mlir.global"() ({}) {sym_name = "foo", global_type = i64, value = 42 : i64, addr_space = -1 : i32, linkage = #llvm.linkage<private>} : () -> ()
 

diff --git a/mlir/test/Dialect/LLVMIR/invalid.mlir b/mlir/test/Dialect/LLVMIR/invalid.mlir
@@ -125,6 +125,13 @@ func.func @gep_too_few_dynamic(%base : !llvm.ptr) {
 
 // -----
 
+func.func @load_non_llvm_type(%foo : memref<f32>) {
+  // expected-error@+1 {{op operand #0 must be LLVM pointer type}}
+  llvm.load %foo : memref<f32> -> f32
+}
+
+// -----
+
 func.func @load_syncscope(%ptr : !llvm.ptr) {
   // expected-error@below {{expected syncscope to be null for non-atomic access}}
   %1 = "llvm.load"(%ptr) {syncscope = "singlethread"} : (!llvm.ptr) -> (f32)