[MLIR][SPIRVToLLVM] Conversion of SPIR-V array, runtime array, pointer and struct types to LLVM dialect.

[georgemitenkov]

Summary

This patch adds type conversion for 4 SPIR-V types: array, runtime array, pointer and struct. This conversion is integrated using a separate function populateSPIRVToLLVMTypeConversion() that adds new type conversions. At the moment, this is a basic skeleton that allows to perfom some basic conversion from SPIR-V to LLVM. There is no support of array strides, struct offset and member decorations, as well as of storage classes. I think that these will be supported on the case by case basis.

[ftynse]

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

491

I'm slightly concerned about this capturing typeConverter reference, even though the lambda is stored into the typeConverter itself so the lifetime is fine. @rriddle anything we can do for type conversions that require recursive calls into the type conversion infra without exposing such things to the user?

[rriddle]

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

491

Capturing the reference is fine so long as we make TypeConverter non-copyable. I don't think it is right now, but that seems like a fine trade-off IMO.

[ftynse]

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

70

Please add some documentation for top-level functions.

In general, I think it would be useful to also have a document like https://mlir.llvm.org/docs/ConversionToLLVMDialect/ (feel free to rename that document to something that mentions it's only for the standard dialect) that explains the SPIRV-to-LLVM conversion on the high level. For example, type conversion rules.

mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm.mlir

42

How would this work for offsets that are not equal to cumulative size of the previous struct elements, e.g. <f32 [0], i32 [8]> ?

[georgemitenkov]

mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm.mlir

42

This is not supported yet, although some padding fields can be inserted to emulate the offset in my opinion.

[georgemitenkov]

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

70

I will put some comments in the code and create a markdown in a separate patch then.

[ftynse]

mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm.mlir

42

Not supported in the conversion, or in the SPIRV dialect? If only in the conversion, I'd suggest to add a check for that and fail the conversion.

LLVM IR can also pad structures, e.g., {i8, i32} will likely have 3 pad bytes between the elements; a packed structure, <{i8, i32}>, won't.

[georgemitenkov]

mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm.mlir

42

In the conversion. SPIR-V actually allows struct members to have offsets that are more than the the size of previous elements (even though it is not a common case as far as I know).

[georgemitenkov]

• [MLIR][SPIRVToLLVM] Added comments to type conversion functions and refactored struct conversion
• [MLIR][SPIRVToLLVM] Refactored the code to follow clang style

Added comments to document type conversion functions. This indicates what is currently supported.
Refactored convertStructType() to only handle structs with no offset. They will be mapped to packed structs. The case when structs have offset will be a separate case (either inserting extra padding or keeping the offset as sizeof(previous elements)) and will be implemented shortly.

[georgemitenkov]

• [MLIR][SPIRVToLLVM] Added missing tests for packed struct

Since struct with no offset are modelled as packed structs, the test had to be updated to reflect this change.

[antiagainst]

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

70

+1. Thanks for the great point, @ftynse! We should have such a doc. Apart from being generally useful for understanding the logic and code, it could also be a nice artifact of the GSOC project itself.

85

Should return null type if type.getArrayStride() != 0 to indicate that's not supported.

105

Same here. Check to reject unsupported cases.

115

I think we should check and return null type to indicate not supported case here. The assert can happen in convertStructTypePacked. It's a subtle difference but generally asserts means we are seeing unexpected input in an API and the programmer is breaking preconditions, which is the case for convertStructTypePacked. For convertStructType, all sorts of struct types should be expected input; it's just that we are not handling them yet. So return naturally to indicate an error would be nice than abort abruptly only under debug mode.

mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm.mlir

42

We should check and return failure during conversion for the unsupported case. Type conversion is quite fundamental to dialect conversion and the infrastructure is quite involved on this front; so being explicit (and verbose) to reject unsupported case can be quite helpful for development. And we should also have negative tests here that check we do nothing for types with offsets.

(As a background: SPIR-V struct types with explicit layout is really for kernel-runtime ABI. And normally you can expect the data are stored in buffers tightly so each element is having natural size and alignment. Most of the time we just see explicit layout like that, esp. for ML I think. But there indeed exist cases where we want to place data differently. It can happen in graphics world, which is not in the GSOC's scope here.)

[georgemitenkov]

• [MLIR][SPIRVToLLVM] Added conversion for naturally padded structs
• [MLIR][SPIRVToLLVM] Added alignment in struct conversion pattern
• [MLIR][SPIRVToLLVM] Added comments to padded struct conversion
• [MLIR][SPIRVToLLVM] Refactored the code to follow clang style

Addressed comments:

1. Array conversions return null if type.getArrayStride() == 0 is not satisfied.
2. Struct type conversion is now splitted in two. It can be converted to a packed struct if there is no offset. If there is an offset, then a check if the offsets are "natural" is performed. At the moment, struct with offset conversion supports scalars, pointers and arrays as its members. I also added tests that indicate a failure of conversion.

[antiagainst]

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

70

I think you can do for (Type elementType : type.getElementTypes) or llvm::map_range(type.getElementTypes(), [...](...) { ... }) here?

91

I feel this is dense logic that worth some more testing and it's better to have a separate commit for it. Could you split this case out?

98

typo: butes

102

Ah, the cast<spirv::SPIRVType> part is annoying. I think we should actually change spirv::StructType's APIs to return SPIRVType instead of Type and hide this inside the APIs, given a SPIR-V struct can only contain SPIR-V types for sure.

[georgemitenkov]

I will split this review in 2 new revisions:

• Adds array. runtime array and pointer conversion
• Adds struct conversion without offset
• Support of structs with natural offset

mlir/include/mlir/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.h

SPIRVToLLVMConversion(MLIRContext *context, LLVMTypeConverter &typeConverter,
                         PatternBenefit benefit = 1)
       : OpConversionPattern<SPIRVOp>(context, benefit),
         typeConverter(typeConverter) {}
 
 protected:
   LLVMTypeConverter &typeConverter;
 };
 
+/// Populates type conversions with additional SPIR-V types.
+void populateSPIRVToLLVMTypeConversion(LLVMTypeConverter &typeConverter);
+
 /// Populates the given list with patterns that convert from SPIR-V to LLVM.
 void populateSPIRVToLLVMConversionPatterns(MLIRContext *context,
                                            LLVMTypeConverter &typeConverter,
                                            OwningRewritePatternList &patterns);
 
 /// Populates the given list with patterns for function conversion from SPIR-V
 /// to LLVM.
 void populateSPIRVToLLVMFunctionConversionPatterns(

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVM.cpp

IntegerAttr minusOneIntegerAttribute(Type type, Builder builder) {
   if (auto vecType = type.dyn_cast<VectorType>()) {
     auto integerType = vecType.getElementType().cast<IntegerType>();
     return builder.getIntegerAttr(integerType, -1);
   }
   auto integerType = type.cast<IntegerType>();
   return builder.getIntegerAttr(integerType, -1);
 }
 
+/// Converts SPIR-V struct with no offset to packed LLVM struct.
+static Type convertStructTypePacked(spirv::StructType type,
+                                    LLVMTypeConverter &converter) {
+  SmallVector<LLVM::LLVMType, 8> elementTypes;
+  for (unsigned i = 0, e = type.getNumElements(); i < e; ++i) {
+    auto convertedType =
+        converter.convertType(type.getElementType(i)).cast<LLVM::LLVMType>();
+    elementTypes.push_back(convertedType);
+  }
+  return LLVM::LLVMType::getStructTy(converter.getDialect(), elementTypes,
+                                     /*isPacked=*/true);
+}
+
+/// Returns the type size in bytes. If the type is an array, returns the size in
+/// bytes of the first element.
+static Optional<int64_t> getElementOrSelfNumBytes(spirv::SPIRVType type) {
+  if (auto arrayType = type.dyn_cast<spirv::ArrayType>())
+    return getElementOrSelfNumBytes(
+        arrayType.getElementType().cast<spirv::SPIRVType>());
+  return type.getTypeNumBytes();
+}
+
+/// Converts SPIR-V struct with offset to LLVM struct. Fails if the struct is
+/// padded unnaturally.
+static Optional<Type>
+convertStructTypeWithOffset(spirv::StructType type,
+                            LLVMTypeConverter &converter) {
+  unsigned numElements = type.getNumElements();
+  if (numElements == 0)
+    return LLVM::LLVMType::getStructTy(converter.getDialect(),
+                                       ArrayRef<LLVM::LLVMType>());
+
+  // Calculate the offset and the size in butes of the first struct member.
+  int64_t predOffset = type.getMemberOffset(0);
+  if (predOffset != 0)
+    return llvm::None;
+  auto predElementType = type.getElementType(0).cast<spirv::SPIRVType>();
+  Optional<int64_t> typeSizePred = predElementType.getTypeNumBytes();
+  if (!typeSizePred)
+    return llvm::None;
+
+  SmallVector<LLVM::LLVMType, 8> elementTypes;
+  elementTypes.push_back(
+      converter.convertType(predElementType).cast<LLVM::LLVMType>());
+  for (unsigned i = 1; i < numElements; ++i) {
+    auto elementType = type.getElementType(i).cast<spirv::SPIRVType>();
+    Optional<int64_t> elementOrSelfSize = getElementOrSelfNumBytes(elementType);
+    if (!elementOrSelfSize)
+      return llvm::None;
+
+    // Check if the offset of struct members is natural.
+    int64_t offset = type.getMemberOffset(i);
+    if (offset % *elementOrSelfSize != 0 ||
+        offset - predOffset - *typeSizePred >= *elementOrSelfSize ||
+        offset - predOffset - *typeSizePred < 0)
+      return llvm::None;
+
+    // If the member is an array, we need to calculate its size that will be
+    // used on next iteration.
+    if (elementType.isa<spirv::ArrayType>()) {
+      Optional<int64_t> fullSize =
+          elementType.cast<spirv::SPIRVType>().getTypeNumBytes();
+      if (!fullSize)
+        return llvm::None;
+      typeSizePred = *fullSize;
+    } else {
+      typeSizePred = *elementOrSelfSize;
+    }
+    predOffset = offset;
+
+    auto convertedType =
+        converter.convertType(elementType).cast<LLVM::LLVMType>();
+    elementTypes.push_back(convertedType);
+  }
+  return LLVM::LLVMType::getStructTy(converter.getDialect(), elementTypes);
+}
+
+//===----------------------------------------------------------------------===//
+// Type conversion
+//===----------------------------------------------------------------------===//
+
+/// Converts SPIR-V array type to LLVM array. There is no modelling of array
+/// stride at the moment.
+static Optional<Type> convertArrayType(spirv::ArrayType type,
+                                       TypeConverter &converter) {
+  if (type.getArrayStride() != 0)
+    return llvm::None;
+  auto elementType =
+      converter.convertType(type.getElementType()).cast<LLVM::LLVMType>();
+  unsigned numElements = type.getNumElements();
+  return LLVM::LLVMType::getArrayTy(elementType, numElements);
+}
+
+/// Converts SPIR-V pointer type to LLVM pointer. Pointer's storage class is not
+/// modelled at the moment.
+static Type convertPointerType(spirv::PointerType type,
+                               TypeConverter &converter) {
+  auto pointeeType =
+      converter.convertType(type.getPointeeType()).cast<LLVM::LLVMType>();
+  return pointeeType.getPointerTo();
+}
+
+/// Converts SPIR-V runtime array to LLVM array. Since LLVM allows indexing over
+/// the bounds, the runtime array is converted to a 0-sized LLVM array. There is
+/// no modelling of array stride at the moment.
+static Optional<Type> convertRuntimeArrayType(spirv::RuntimeArrayType type,
+                                              TypeConverter &converter) {
+  if (type.getArrayStride() != 0)
+    return llvm::None;
+  auto elementType =
+      converter.convertType(type.getElementType()).cast<LLVM::LLVMType>();
+  return LLVM::LLVMType::getArrayTy(elementType, 0);
+}
+
+/// Converts SPIR-V struct to LLVM struct. There is no support of struct member
+/// decorations. Only "natural" offsets are supported.
+static Optional<Type> convertStructType(spirv::StructType type,
+                                        LLVMTypeConverter &converter) {
+  if (type.hasOffset())
+    return convertStructTypeWithOffset(type, converter);
+  return convertStructTypePacked(type, converter);
+}
+
 //===----------------------------------------------------------------------===//
 // Operation conversion
 //===----------------------------------------------------------------------===//
 
 namespace {
 
 /// Converts SPIR-V operations that have straightforward LLVM equivalent
 /// into LLVM dialect operations.
matchAndRewrite(spirv::FuncOp funcOp, ArrayRef<Value> operands,
 
     // Convert function signature. At the moment LLVMType converter is enough
     // for currently supported types.
     auto funcType = funcOp.getType();
     TypeConverter::SignatureConversion signatureConverter(
         funcType.getNumInputs());
     auto llvmType = this->typeConverter.convertFunctionSignature(
         funcOp.getType(), /*isVariadic=*/false, signatureConverter);
+    if (!llvmType)
+      return failure();
 
     // Create a new `LLVMFuncOp`
     Location loc = funcOp.getLoc();
     StringRef name = funcOp.getName();
     auto newFuncOp = rewriter.create<LLVM::LLVMFuncOp>(loc, name, llvmType);
 
     // Convert SPIR-V Function Control to equivalent LLVM function attribute
     MLIRContext *context = funcOp.getContext();

 };
 
 } // namespace
 
 //===----------------------------------------------------------------------===//
 // Pattern population
 //===----------------------------------------------------------------------===//
 
+void mlir::populateSPIRVToLLVMTypeConversion(LLVMTypeConverter &typeConverter) {
+  typeConverter.addConversion([&](spirv::ArrayType type) {
+    return convertArrayType(type, typeConverter);
+  });
+  typeConverter.addConversion([&](spirv::PointerType type) {
+    return convertPointerType(type, typeConverter);
+  });
+  typeConverter.addConversion([&](spirv::RuntimeArrayType type) {
+    return convertRuntimeArrayType(type, typeConverter);
+  });
+  typeConverter.addConversion([&](spirv::StructType type) {
+    return convertStructType(type, typeConverter);
+  });
+}
+
 void mlir::populateSPIRVToLLVMConversionPatterns(
     MLIRContext *context, LLVMTypeConverter &typeConverter,
     OwningRewritePatternList &patterns) {
   patterns.insert<
       // Arithmetic ops
       DirectConversionPattern<spirv::IAddOp, LLVM::AddOp>,
       DirectConversionPattern<spirv::IMulOp, LLVM::MulOp>,
       DirectConversionPattern<spirv::ISubOp, LLVM::SubOp>,

mlir/lib/Conversion/SPIRVToLLVM/ConvertSPIRVToLLVMPass.cpp

 } // namespace
 
 void ConvertSPIRVToLLVMPass::runOnOperation() {
   MLIRContext *context = &getContext();
   ModuleOp module = getOperation();
   LLVMTypeConverter converter(&getContext());
 
   OwningRewritePatternList patterns;
+
+  populateSPIRVToLLVMTypeConversion(converter);
+
   populateSPIRVToLLVMModuleConversionPatterns(context, converter, patterns);
   populateSPIRVToLLVMConversionPatterns(context, converter, patterns);
   populateSPIRVToLLVMFunctionConversionPatterns(context, converter, patterns);
 
   // Currently pulls in Std to LLVM conversion patterns
   // that help with testing. This allows to convert
   // function arguments to LLVM.
   populateStdToLLVMConversionPatterns(converter, patterns);

mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm-invalid.mlir

+// RUN: mlir-opt %s -convert-spirv-to-llvm -verify-diagnostics -split-input-file
+
+// expected-error@+1 {{failed to legalize operation 'spv.func' that was explicitly marked illegal}}
+spv.func @array_with_stride(%arg: !spv.array<4 x f32, stride=4>) -> () "None" {
+  spv.Return
+}
+
+// -----
+
+// expected-error@+1 {{failed to legalize operation 'spv.func' that was explicitly marked illegal}}
+spv.func @nested_struct_with_offset(%arg: !spv.struct<!spv.struct<i64[0], i8[8]>[0], i8[16]>) -> () "None" {
+  spv.Return
+}
+
+// -----
+
+// expected-error@+1 {{failed to legalize operation 'spv.func' that was explicitly marked illegal}}
+spv.func @struct_with_unnatural_offset1(%arg: !spv.struct<i32[0], i32[8]>) -> () "None" {
+  spv.Return
+}
+
+// -----
+
+// expected-error@+1 {{failed to legalize operation 'spv.func' that was explicitly marked illegal}}
+spv.func @struct_with_unnatural_offset2(%arg: !spv.struct<i32[2], i32[6]>) -> () "None" {
+  spv.Return
+}
+
+// -----
+
+// expected-error@+1 {{failed to legalize operation 'spv.func' that was explicitly marked illegal}}
+spv.func @struct_with_unnatural_offset3(%arg: !spv.struct<i32[0], i32[2]>) -> () "None" {
+  spv.Return
+}

mlir/test/Conversion/SPIRVToLLVM/spirv-types-to-llvm.mlir

+// RUN: mlir-opt -split-input-file -convert-spirv-to-llvm -verify-diagnostics %s | FileCheck %s
+
+//===----------------------------------------------------------------------===//
+// Array type
+//===----------------------------------------------------------------------===//
+
+// CHECK-LABEL: @array(!llvm<"[16 x float]">, !llvm<"[32 x <4 x float>]">)
+func @array(!spv.array<16xf32>, !spv.array< 32 x vector<4xf32> >) -> ()
+
+//===----------------------------------------------------------------------===//
+// Pointer type
+//===----------------------------------------------------------------------===//
+
+// CHECK-LABEL: @pointer_scalar(!llvm<"i1*">, !llvm<"float*">)
+func @pointer_scalar(!spv.ptr<i1, Uniform>, !spv.ptr<f32, Private>) -> ()
+
+// CHECK-LABEL: @pointer_vector(!llvm<"<4 x i32>*">)
+func @pointer_vector(!spv.ptr<vector<4xi32>, Function>) -> ()
+
+//===----------------------------------------------------------------------===//
+// Runtime array type
+//===----------------------------------------------------------------------===//
+
+// CHECK-LABEL: @runtime_array_vector(!llvm<"[0 x <4 x float>]">)
+func @runtime_array_vector(!spv.rtarray< vector<4xf32> >) -> ()
+
+// CHECK-LABEL: @runtime_array_scalar(!llvm<"[0 x float]">)
+func @runtime_array_scalar(!spv.rtarray<f32>) -> ()
+
+//===----------------------------------------------------------------------===//
+// Struct type
+//===----------------------------------------------------------------------===//
+
+// CHECK-LABEL: @struct(!llvm<"<{ double }>">)
+func @struct(!spv.struct<f64>) -> ()
+
+// CHECK-LABEL: @struct_nested(!llvm<"<{ i32, <{ i64, i32 }> }>">)
+func @struct_nested(!spv.struct<i32, !spv.struct<i64, i32>>)
+
+// CHECK-LABEL: llvm.func @struct_with_decoration(!llvm<"<{ float }>">)
+func @struct_with_decoration(!spv.struct<f32 [RelaxedPrecision]>)
+
+// CHECK-LABEL: llvm.func @struct_with_offset1(!llvm<"{ i8, i64, i32 }">)
+func @struct_with_offset1(!spv.struct<i8[0], i64[8], i32[16]>)
+
+// CHECK-LABEL: llvm.func @struct_with_offset2(!llvm<"{ [5 x i8], i32, i8, i8 }">)
+func @struct_with_offset2(!spv.struct<!spv.array<5xi8>[0], i32[8], i8[12], i8[13]>)