[mlir] recursively convert builtin types to LLVM when possible

[ftynse]

Summary

Given that LLVM dialect types may now optionally contain types from other
dialects, which itself is motivated by dialect interoperability and progressive
lowering, the conversion should no longer assume that the outermost LLVM
dialect type can be left as is. Instead, it should inspect the types it
contains and attempt to convert them to the LLVM dialect. Introduce this
capability for LLVM array, pointer and structure types. Only literal structures
are currently supported as handling identified structures requires the
converison infrastructure to have a mechanism for avoiding infite recursion in
case of recursive types.

[ftynse]

Now with a test.

[rriddle]

mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp

45

Leftover debugging?

61–67

Is there a reason not to use convertTypes?

82–88

Same here.

[ftynse]

@rriddle, since this grabbed your attention, any thoughts on how we should handle recursive types in the conversion? Would local stack somewhere in TypeConverter be enough?

[ftynse]

Address review.

[ftynse]

mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp

61–67

I forgot it existed? :)

[rriddle]

In D112550#3088698, @ftynse wrote:

@rriddle, since this grabbed your attention, any thoughts on how we should handle recursive types in the conversion? Would local stack somewhere in TypeConverter be enough?

I think adding a set/stack to TypeConverter to track in-progress conversions is fine (we already have maps for cached conversions). The thing that isn't clear to me is how this is exposed to the user, i.e. how does the user detect when they are mid conversion? Do you have any ideas here?

[ftynse]

In D112550#3091639, @rriddle wrote:

In D112550#3088698, @ftynse wrote:

@rriddle, since this grabbed your attention, any thoughts on how we should handle recursive types in the conversion? Would local stack somewhere in TypeConverter be enough?

I think adding a set/stack to TypeConverter to track in-progress conversions is fine (we already have maps for cached conversions). The thing that isn't clear to me is how this is exposed to the user, i.e. how does the user detect when they are mid conversion? Do you have any ideas here?

We could pass the stack as an additional const reference argument to the user-supplied converter function + have a default wrapper that ignores the stack and calls user-supplied functions without this argument.

[rriddle]

In D112550#3092597, @ftynse wrote:

In D112550#3091639, @rriddle wrote:

In D112550#3088698, @ftynse wrote:

@rriddle, since this grabbed your attention, any thoughts on how we should handle recursive types in the conversion? Would local stack somewhere in TypeConverter be enough?

I think adding a set/stack to TypeConverter to track in-progress conversions is fine (we already have maps for cached conversions). The thing that isn't clear to me is how this is exposed to the user, i.e. how does the user detect when they are mid conversion? Do you have any ideas here?

We could pass the stack as an additional const reference argument to the user-supplied converter function + have a default wrapper that ignores the stack and calls user-supplied functions without this argument.

That could work. Would it be better to have a different callback for the recursive case? i.e. detect recursion and then call the other callback? I'm not sure the impact of that on performance though.

[ftynse]

In D112550#3094633, @rriddle wrote:

In D112550#3092597, @ftynse wrote:

In D112550#3091639, @rriddle wrote:

In D112550#3088698, @ftynse wrote:

@rriddle, since this grabbed your attention, any thoughts on how we should handle recursive types in the conversion? Would local stack somewhere in TypeConverter be enough?

I think adding a set/stack to TypeConverter to track in-progress conversions is fine (we already have maps for cached conversions). The thing that isn't clear to me is how this is exposed to the user, i.e. how does the user detect when they are mid conversion? Do you have any ideas here?

We could pass the stack as an additional const reference argument to the user-supplied converter function + have a default wrapper that ignores the stack and calls user-supplied functions without this argument.

That could work. Would it be better to have a different callback for the recursive case? i.e. detect recursion and then call the other callback? I'm not sure the impact of that on performance though.

It is probably worth trying the two to check the performance implications.

Another alternative, which will only work if we require callbacks to be stateless, is to directly cache the result of each call and take it from cache instead of calling the callback when we hit the recursive case.

mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp

LLVMTypeConverter::LLVMTypeConverter(MLIRContext *ctx,
   addConversion([&](FunctionType type) { return convertFunctionType(type); });
   addConversion([&](IndexType type) { return convertIndexType(type); });
   addConversion([&](IntegerType type) { return convertIntegerType(type); });
   addConversion([&](MemRefType type) { return convertMemRefType(type); });
   addConversion(
       [&](UnrankedMemRefType type) { return convertUnrankedMemRefType(type); });
   addConversion([&](VectorType type) { return convertVectorType(type); });
 
-  // LLVM-compatible types are legal, so add a pass-through conversion.
+  // LLVM-compatible types are legal, so add a pass-through conversion. Do this
+  // before the conversions below since conversions are attempted in reverse
+  // order and those should take priority.
   addConversion([](Type type) {
     return LLVM::isCompatibleType(type) ? llvm::Optional<Type>(type)
                                         : llvm::None;
   });
 
+  // LLVM container types may (recursively) contain other types that must be
+  // converted even when the outer type is compatible.
+  addConversion([&](LLVM::LLVMPointerType type) -> llvm::Optional<Type> {
+    if (auto pointee = convertType(type.getElementType()))
+      return LLVM::LLVMPointerType::get(pointee, type.getAddressSpace());
+    return llvm::None;
+  });
+  addConversion([&](LLVM::LLVMStructType type) -> llvm::Optional<Type> {
+    // TODO: handle conversion of identified structs, which may be recursive.
+    if (type.isIdentified())
+      return type;
+
+    SmallVector<Type> convertedSubtypes;
+    convertedSubtypes.reserve(type.getBody().size());
+    if (failed(convertTypes(type.getBody(), convertedSubtypes)))
+      return llvm::None;
+
+    return LLVM::LLVMStructType::getLiteral(type.getContext(),
+                                            convertedSubtypes, type.isPacked());
+  });
+  addConversion([&](LLVM::LLVMArrayType type) -> llvm::Optional<Type> {
+    if (auto element = convertType(type.getElementType()))
+      return LLVM::LLVMArrayType::get(element, type.getNumElements());
+    return llvm::None;
+  });
+  addConversion([&](LLVM::LLVMFunctionType type) -> llvm::Optional<Type> {
+    Type convertedResType = convertType(type.getReturnType());
+    if (!convertedResType)
+      return llvm::None;
+
+    SmallVector<Type> convertedArgTypes;
+    convertedArgTypes.reserve(type.getNumParams());
+    if (failed(convertTypes(type.getParams(), convertedArgTypes)))
+      return llvm::None;
+
+    return LLVM::LLVMFunctionType::get(convertedResType, convertedArgTypes,
+                                       type.isVarArg());
+  });
+
   // Materialization for memrefs creates descriptor structs from individual
   // values constituting them, when descriptors are used, i.e. more than one
   // value represents a memref.
   addArgumentMaterialization(
       [&](OpBuilder &builder, UnrankedMemRefType resultType, ValueRange inputs,
           Location loc) -> Optional<Value> {
         if (inputs.size() == 1)
           return llvm::None;
         return UnrankedMemRefDescriptor::pack(builder, loc, *this, resultType,

mlir/test/Conversion/StandardToLLVM/convert-types.mlir

+// RUN: mlir-opt -test-convert-call-op %s | FileCheck %s
+
+// CHECK-LABEL: @ptr
+// CHECK: !llvm.ptr<i42>
+func private @ptr() -> !llvm.ptr<!test.smpla>
+
+// CHECK-LABEL: @ptr_ptr()
+// CHECK: !llvm.ptr<ptr<i42>> 
+func private @ptr_ptr() -> !llvm.ptr<!llvm.ptr<!test.smpla>>
+
+// CHECK-LABEL: @struct_ptr()
+// CHECK: !llvm.struct<(ptr<i42>)> 
+func private @struct_ptr() -> !llvm.struct<(ptr<!test.smpla>)>
+
+// CHECK-LABEL: @named_struct_ptr()
+// CHECK: !llvm.struct<"named", (ptr<!test.smpla>)> 
+func private @named_struct_ptr() -> !llvm.struct<"named", (ptr<!test.smpla>)>
+
+// CHECK-LABEL: @array_ptr()
+// CHECK: !llvm.array<10 x ptr<i42>> 
+func private @array_ptr() -> !llvm.array<10 x ptr<!test.smpla>>
+
+// CHECK-LABEL: @func()
+// CHECK: !llvm.ptr<func<i42 (i42)>>
+func private @func() -> !llvm.ptr<!llvm.func<!test.smpla (!test.smpla)>>
+
+// TODO: support conversion of recursive types in the conversion infra.
+// CHECK-LABEL: @named_recursive()
+// CHECK: !llvm.struct<"recursive", (ptr<!test.smpla>, ptr<struct<"recursive">>)> 
+func private @named_recursive() -> !llvm.struct<"recursive", (ptr<!test.smpla>, ptr<struct<"recursive">>)>
+

mlir/test/lib/Conversion/StandardToLLVM/TestConvertCallOp.cpp

public:
   void runOnOperation() override {
     ModuleOp m = getOperation();
 
     // Populate type conversions.
     LLVMTypeConverter typeConverter(m.getContext());
     typeConverter.addConversion([&](test::TestType type) {
       return LLVM::LLVMPointerType::get(IntegerType::get(m.getContext(), 8));
     });
+    typeConverter.addConversion([&](test::SimpleAType type) {
+      return IntegerType::get(type.getContext(), 42);
+    });
 
     // Populate patterns.
     RewritePatternSet patterns(m.getContext());
     populateStdToLLVMConversionPatterns(typeConverter, patterns);
     patterns.add<TestTypeProducerOpConverter>(typeConverter);
 
     // Set target.
     ConversionTarget target(getContext());