[mlir][GPU][NFC] Remove type converter hack

[springerm]

Summary

Remove dangerousSetOptions and call promoteOperands with the correct arguments directly.

[springerm]

mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp

984–990

I'm not sure if this is the correct behavior. But it was already like that before this revision.

[herhut]

mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp

984–990

It indeed feels wrong, especially given the name of the parameter. Maybe it should be called 'alwaysUseBarePtrCallConv' to make this clearer, as it is an override. @ftynse do you have context in this?

But this is not a problem for this change.

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

public:
   /// Returns the MLIR context.
   MLIRContext &getContext() const;
 
   /// Returns the LLVM dialect.
   LLVM::LLVMDialect *getDialect() const { return llvmDialect; }
 
   const LowerToLLVMOptions &getOptions() const { return options; }
 
-  /// Set the lowering options to `newOptions`. Note: using this after some
-  /// some conversions have been performed can lead to inconsistencies in the
-  /// IR.
-  void dangerousSetOptions(LowerToLLVMOptions newOptions) {
-    options = std::move(newOptions);
-  }
-
   /// Promote the LLVM representation of all operands including promoting MemRef
   /// descriptors to stack and use pointers to struct to avoid the complexity
   /// of the platform-specific C/C++ ABI lowering related to struct argument
   /// passing.
   SmallVector<Value, 4> promoteOperands(Location loc, ValueRange opOperands,
                                         ValueRange operands, OpBuilder &builder,
                                         bool useBarePtrCallConv = false) const;
 

mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp

 //   llvm.store parameters[i], %fieldPtr
 //   %elementPtr = llvm.getelementptr %array[i]
 //   llvm.store %fieldPtr, %elementPtr
 // return %array
 Value ConvertLaunchFuncOpToGpuRuntimeCallPattern::generateParamsArray(
     gpu::LaunchFuncOp launchOp, OpAdaptor adaptor, OpBuilder &builder) const {
   auto loc = launchOp.getLoc();
   auto numKernelOperands = launchOp.getNumKernelOperands();
-  SmallVector<Value, 4> arguments;
-  if (kernelBarePtrCallConv) {
-    // Hack the bare pointer value on just for the argument promotion
-    const LLVMTypeConverter *converter = getTypeConverter();
-    LowerToLLVMOptions options = converter->getOptions();
-    LowerToLLVMOptions overrideToMatchKernelOpts = options;
-    overrideToMatchKernelOpts.useBarePtrCallConv = true;
-    LLVMTypeConverter newConverter = *converter;
-    newConverter.dangerousSetOptions(overrideToMatchKernelOpts);
-    arguments = newConverter.promoteOperands(
-        loc, launchOp.getOperands().take_back(numKernelOperands),
-        adaptor.getOperands().take_back(numKernelOperands), builder);
-  } else {
-    arguments = getTypeConverter()->promoteOperands(
-        loc, launchOp.getOperands().take_back(numKernelOperands),
-        adaptor.getOperands().take_back(numKernelOperands), builder);
-  }
-
+  // Note: If `useBarePtrCallConv` is set in the type converter's options,
+  // the value of `kernelBarePtrCallConv` will be ignored.
+  SmallVector<Value, 4> arguments = getTypeConverter()->promoteOperands(
+      adaptor.getOperands().take_back(numKernelOperands), builder,
+      /*useBarePtrCallConv=*/kernelBarePtrCallConv);
   auto numArguments = arguments.size();
   SmallVector<Type, 4> argumentTypes;
   argumentTypes.reserve(numArguments);
   for (auto argument : arguments)
     argumentTypes.push_back(argument.getType());
   auto structType = LLVM::LLVMStructType::getNewIdentified(context, StringRef(),
                                                            argumentTypes);
   auto one = builder.create<LLVM::ConstantOp>(loc, llvmInt32Type, 1);
   auto structPtr = builder.create<LLVM::AllocaOp>(
if (ArrayAttr targets = kernelModule.getTargetsAttr()) {
     if (adaptor.getAsyncDependencies().size())
       stream = adaptor.getAsyncDependencies().front();
     // If the async keyword is present and there are no dependencies, then a
     // stream must be created to pass to subsequent operations.
     else if (launchOp.getAsyncToken())
       stream = streamCreateCallBuilder.create(loc, rewriter, {}).getResult();
 
     // Lower the kernel operands to match kernel parameters.
-    SmallVector<Value, 4> arguments;
-    if (kernelBarePtrCallConv) {
-      // Hack the bare pointer value on just for the argument promotion
-      const LLVMTypeConverter *converter = getTypeConverter();
-      LowerToLLVMOptions options = converter->getOptions();
-      LowerToLLVMOptions overrideToMatchKernelOpts = options;
-      overrideToMatchKernelOpts.useBarePtrCallConv = true;
-      LLVMTypeConverter newConverter = *converter;
-      newConverter.dangerousSetOptions(overrideToMatchKernelOpts);
-      arguments =
-          newConverter.promoteOperands(loc, launchOp.getKernelOperands(),
-                                       adaptor.getKernelOperands(), rewriter);
-    } else {
-      arguments = getTypeConverter()->promoteOperands(
-          loc, launchOp.getKernelOperands(), adaptor.getKernelOperands(),
-          rewriter);
-    }
+    // Note: If `useBarePtrCallConv` is set in the type converter's options,
+    // the value of `kernelBarePtrCallConv` will be ignored.
+    SmallVector<Value, 4> arguments = getTypeConverter()->promoteOperands(
+        rewriter, /*useBarePtrCallConv=*/kernelBarePtrCallConv);
 
     rewriter.create<gpu::LaunchFuncOp>(
         launchOp.getLoc(), launchOp.getKernelAttr(),
         gpu::KernelDim3{adaptor.getGridSizeX(), adaptor.getGridSizeY(),
                         adaptor.getGridSizeZ()},
         gpu::KernelDim3{adaptor.getBlockSizeX(), adaptor.getBlockSizeY(),
                         adaptor.getBlockSizeZ()},
         adaptor.getDynamicSharedMemorySize(), arguments, stream);