[Mlir-commits] [mlir] 30ca16e - [mlir][spirv] Handle failed conversions of struct elements (#70005)

[llvmlistbot at llvm.org]

Author: Pierre van Houtryve
Date: 2023-10-30T07:48:24+01:00
New Revision: 30ca16ec87206294f4ad0e9688c88f32421b343e

URL: https://github.com/llvm/llvm-project/commit/30ca16ec87206294f4ad0e9688c88f32421b343e
DIFF: https://github.com/llvm/llvm-project/commit/30ca16ec87206294f4ad0e9688c88f32421b343e.diff

LOG: [mlir][spirv] Handle failed conversions of struct elements (#70005)

LLVMStructTypes could be emitted with some null elements. This caused a
crash later in the LLVMDialect verifier.

We now use `convertTypes` and check that all types were successfully converted before passing them to the `LLVMStructType` constructor.

See #59990

Added: 
    

Modified: 
    mlir/include/mlir/Dialect/SPIRV/IR/SPIRVTypes.h
    mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
    mlir/lib/Dialect/SPIRV/IR/SPIRVTypes.cpp
    mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm-invalid.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVTypes.h b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVTypes.h
index 07f2f158ecabb6f..4be2582f8fd68cc 100644
--- a/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVTypes.h
+++ b/mlir/include/mlir/Dialect/SPIRV/IR/SPIRVTypes.h
@@ -347,27 +347,7 @@ class StructType
 
   Type getElementType(unsigned) const;
 
-  /// Range class for element types.
-  class ElementTypeRange
-      : public ::llvm::detail::indexed_accessor_range_base<
-            ElementTypeRange, const Type *, Type, Type, Type> {
-  private:
-    using RangeBaseT::RangeBaseT;
-
-    /// See `llvm::detail::indexed_accessor_range_base` for details.
-    static const Type *offset_base(const Type *object, ptr
diff _t index) {
-      return object + index;
-    }
-    /// See `llvm::detail::indexed_accessor_range_base` for details.
-    static Type dereference_iterator(const Type *object, ptr
diff _t index) {
-      return object[index];
-    }
-
-    /// Allow base class access to `offset_base` and `dereference_iterator`.
-    friend RangeBaseT;
-  };
-
-  ElementTypeRange getElementTypes() const;
+  TypeRange getElementTypes() const;
 
   bool hasOffset() const;
 

diff  --git a/mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp b/mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
index 60f34f413f587d4..0ea82730c065a7c 100644
--- a/mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
+++ b/mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp
@@ -201,16 +201,14 @@ static Value processCountOrOffset(Location loc, Value value, Type srcType,
 
 /// Converts SPIR-V struct with a regular (according to `VulkanLayoutUtils`)
 /// offset to LLVM struct. Otherwise, the conversion is not supported.
-static std::optional<Type>
-convertStructTypeWithOffset(spirv::StructType type,
-                            LLVMTypeConverter &converter) {
+static Type convertStructTypeWithOffset(spirv::StructType type,
+                                        LLVMTypeConverter &converter) {
   if (type != VulkanLayoutUtils::decorateType(type))
-    return std::nullopt;
+    return nullptr;
 
-  auto elementsVector = llvm::to_vector<8>(
-      llvm::map_range(type.getElementTypes(), [&](Type elementType) {
-        return converter.convertType(elementType);
-      }));
+  SmallVector<Type> elementsVector;
+  if (failed(converter.convertTypes(type.getElementTypes(), elementsVector)))
+    return nullptr;
   return LLVM::LLVMStructType::getLiteral(type.getContext(), elementsVector,
                                           /*isPacked=*/false);
 }
@@ -218,10 +216,9 @@ convertStructTypeWithOffset(spirv::StructType type,
 /// Converts SPIR-V struct with no offset to packed LLVM struct.
 static Type convertStructTypePacked(spirv::StructType type,
                                     LLVMTypeConverter &converter) {
-  auto elementsVector = llvm::to_vector<8>(
-      llvm::map_range(type.getElementTypes(), [&](Type elementType) {
-        return converter.convertType(elementType);
-      }));
+  SmallVector<Type> elementsVector;
+  if (failed(converter.convertTypes(type.getElementTypes(), elementsVector)))
+    return nullptr;
   return LLVM::LLVMStructType::getLiteral(type.getContext(), elementsVector,
                                           /*isPacked=*/true);
 }
@@ -335,12 +332,12 @@ static std::optional<Type> convertRuntimeArrayType(spirv::RuntimeArrayType type,
 
 /// Converts SPIR-V struct to LLVM struct. There is no support of structs with
 /// member decorations. Also, only natural offset is supported.
-static std::optional<Type> convertStructType(spirv::StructType type,
-                                             LLVMTypeConverter &converter) {
+static Type convertStructType(spirv::StructType type,
+                              LLVMTypeConverter &converter) {
   SmallVector<spirv::StructType::MemberDecorationInfo, 4> memberDecorations;
   type.getMemberDecorations(memberDecorations);
   if (!memberDecorations.empty())
-    return std::nullopt;
+    return nullptr;
   if (type.hasOffset())
     return convertStructTypeWithOffset(type, converter);
   return convertStructTypePacked(type, converter);

diff  --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVTypes.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVTypes.cpp
index 39d6603a46f965d..f1bac6490837b93 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVTypes.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVTypes.cpp
@@ -1146,9 +1146,9 @@ Type StructType::getElementType(unsigned index) const {
   return getImpl()->memberTypesAndIsBodySet.getPointer()[index];
 }
 
-StructType::ElementTypeRange StructType::getElementTypes() const {
-  return ElementTypeRange(getImpl()->memberTypesAndIsBodySet.getPointer(),
-                          getNumElements());
+TypeRange StructType::getElementTypes() const {
+  return TypeRange(getImpl()->memberTypesAndIsBodySet.getPointer(),
+                   getNumElements());
 }
 
 bool StructType::hasOffset() const { return getImpl()->offsetInfo; }

diff  --git a/mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm-invalid.mlir b/mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm-invalid.mlir
index 3965c47ec199fcb..438c90205abedc4 100644
--- a/mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm-invalid.mlir
+++ b/mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm-invalid.mlir
@@ -7,6 +7,13 @@ spirv.func @array_with_unnatural_stride(%arg: !spirv.array<4 x f32, stride=8>) -
 
 // -----
 
+// expected-error at +1 {{failed to legalize operation 'spirv.func' that was explicitly marked illegal}}
+spirv.func @struct_array_with_unnatural_stride(%arg: !spirv.struct<(!spirv.array<4 x f32, stride=8>)>) -> () "None" {
+  spirv.Return
+}
+
+// -----
+
 // expected-error at +1 {{failed to legalize operation 'spirv.func' that was explicitly marked illegal}}
 spirv.func @struct_with_unnatural_offset(%arg: !spirv.struct<(i32[0], i32[8])>) -> () "None" {
   spirv.Return