[mlir] LLVM lowering: don't use calling convention in op returns

[ftynse]

Summary

Conversions to the LLVM dialect have an option to use the "bare pointer"
calling convention that converts memref types differently than the
default convention. It has crept into the conversion of operations that
are not related to calls but do require multiresult-to-struct packing.
Use a similar mechanism for the latter without using the calling
convention.

[gysit]

LGTM, would it be NFC since there are no test modifications needed?

[ftynse]

I don't have a good way to test this. The issue is only exposed if you pass an LLVMTypeConverter configured for the bare-pointer convention into populateArithToLLVMConversionPatterns, which our current passes don't do and probably don't want to do. Any suggestion (short of rolling a test pass that combines patterns)?

[gysit]

I don't have a good way to test this. The issue is only exposed if you pass an LLVMTypeConverter configured for the bare-pointer convention into populateArithToLLVMConversionPatterns, which our current passes don't do and probably don't want to do. Any suggestion (short of rolling a test pass that combines patterns)?

At least the standard use case is tested but not the corner case you are fixing?

I am surprised the Arith to LLVM lowering can go wrong. Isn't the difference the memref result handling only?

I would also assume a test conversion pass is probably the "easiest" way to test the behavior if you want to add a test. However, it seems like a lot of effort for a quite exotic use case?

[ftynse]

Indeed, this is relevant only for memrefs. Specifically, I have a downstream client that fails when lowering arith.select : mermef<42xf32> with bare pointers because it rolls a custom pass mixing multiple patterns.

mlir/include/mlir/Conversion/LLVMCommon/TypeConverter.h

public:
 
   /// Convert a function type.  The arguments and results are converted one by
   /// one and results are packed into a wrapped LLVM IR structure type. `result`
   /// is populated with argument mapping.
   Type convertFunctionSignature(FunctionType funcTy, bool isVariadic,
                                 bool useBarePtrCallConv,
                                 SignatureConversion &result);
 
-  /// Convert a non-empty list of types to be returned from a function into a
-  /// supported LLVM IR type.  In particular, if more than one value is
-  /// returned, create an LLVM IR structure type with elements that correspond
-  /// to each of the MLIR types converted with `convertType`.
+  /// Convert a non-empty list of types to be returned from a function into an
+  /// LLVM-compatible type. In particular, if more than one value is returned,
+  /// create an LLVM dialect structure type with elements that correspond to
+  /// each of the types converted with `convertCallingConventionType`.
   Type packFunctionResults(TypeRange types,
                            bool useBarePointerCallConv = false);
 
+  /// Convert a non-empty list of types of values produced by an operation into
+  /// an LLVM-compatible type. In particular, if more than one value is
+  /// produced, create a literal structure with elements that correspond to each
+  /// of the LLVM-compatible types converted with `convertType`.
+  Type packOperationResults(TypeRange types);
+
   /// Convert a type in the context of the default or bare pointer calling
   /// convention. Calling convention sensitive types, such as MemRefType and
   /// UnrankedMemRefType, are converted following the specific rules for the
   /// calling convention. Calling convention independent types are converted
   /// following the default LLVM type conversions.
   Type convertCallingConventionType(Type type,
                                     bool useBarePointerCallConv = false);
 

mlir/lib/Conversion/LLVMCommon/Pattern.cpp

LogicalResult LLVM::detail::oneToOneRewrite(
     Operation *op, StringRef targetOp, ValueRange operands,
     ArrayRef<NamedAttribute> targetAttrs, LLVMTypeConverter &typeConverter,
     ConversionPatternRewriter &rewriter) {
   unsigned numResults = op->getNumResults();
 
   SmallVector<Type> resultTypes;
   if (numResults != 0) {
     resultTypes.push_back(
-        typeConverter.packFunctionResults(op->getResultTypes()));
+        typeConverter.packOperationResults(op->getResultTypes()));
     if (!resultTypes.back())
       return failure();
   }
 
   // Create the operation through state since we don't know its C++ type.
   Operation *newOp =
       rewriter.create(op->getLoc(), rewriter.getStringAttr(targetOp), operands,
                       resultTypes, targetAttrs);

mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp

void LLVMTypeConverter::promoteBarePtrsToDescriptors(
   assert(stdTypes.size() == values.size() &&
          "The number of types and values doesn't match");
   for (unsigned i = 0, end = values.size(); i < end; ++i)
     if (auto memrefTy = stdTypes[i].dyn_cast<MemRefType>())
       values[i] = MemRefDescriptor::fromStaticShape(rewriter, loc, *this,
                                                     memrefTy, values[i]);
 }
 
-/// Convert a non-empty list of types to be returned from a function into a
-/// supported LLVM IR type.  In particular, if more than one value is returned,
-/// create an LLVM IR structure type with elements that correspond to each of
-/// the MLIR types converted with `convertType`.
+/// Convert a non-empty list of types of values produced by an operation into an
+/// LLVM-compatible type. In particular, if more than one value is
+/// produced, create a literal structure with elements that correspond to each
+/// of the types converted with `convertType`.
+Type LLVMTypeConverter::packOperationResults(TypeRange types) {
+  assert(!types.empty() && "expected non-empty list of type");
+  if (types.size() == 1)
+    return convertType(types[0]);
+
+  SmallVector<Type> resultTypes;
+  resultTypes.reserve(types.size());
+  for (Type type : types) {
+    Type converted = convertType(type);
+    if (!converted || !LLVM::isCompatibleType(converted))
+      return {};
+    resultTypes.push_back(converted);
+  }
+
+  return LLVM::LLVMStructType::getLiteral(&getContext(), resultTypes);
+}
+
+/// Convert a non-empty list of types to be returned from a function into an
+/// LLVM-compatible type. In particular, if more than one value is returned,
+/// create an LLVM dialect structure type with elements that correspond to each
+/// of the types converted with `convertCallingConventionType`.
 Type LLVMTypeConverter::packFunctionResults(TypeRange types,
                                             bool useBarePtrCallConv) {
   assert(!types.empty() && "expected non-empty list of type");
 
   useBarePtrCallConv |= options.useBarePtrCallConv;
   if (types.size() == 1)
     return convertCallingConventionType(types.front(), useBarePtrCallConv);
 
-  SmallVector<Type, 8> resultTypes;
+  SmallVector<Type> resultTypes;
   resultTypes.reserve(types.size());
   for (auto t : types) {
     auto converted = convertCallingConventionType(t, useBarePtrCallConv);
     if (!converted || !LLVM::isCompatibleType(converted))
       return {};
     resultTypes.push_back(converted);
   }