[mlir] Drop customization hooks from StandardToLLVM conversion

[ftynse]

Summary

These hooks were originally introduced to support passes deriving the
StandardToLLVM conversion, in particular converting types from different
dialects to LLVM types in a single-step conversion. They are no longer in use
since the pass and conversion infrastructure has evolved sufficiently to make
defining new passes with exactly the same functionality simple through the use
of populate* functions, conversion targets and type converters. Remove the
hooks. Any users of this hooks can call the dialect conversion infrastructure
directly instead, which is likely to require less LoC than these hooks.

[merge_guards_bot]

Unit tests: pass. 62372 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[rriddle]

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h

17–18

nit: Can we just remove this comment now?

[nicolasvasilache]

Nice cleanup, thanks Alex!

[dcaballe]

Alex, Thanks for this! It will definitely make things simpler! However, it's going to conflict with D72802. Could we wait for that diff to land and see what else is needed here to keep everything?

[ftynse]

In D73795#1852442, @dcaballe wrote:

Alex, Thanks for this! It will definitely make things simpler! However, it's going to conflict with D72802. Could we wait for that diff to land and see what else is needed here to keep everything?

Sure, I'll wait for you to land and will rebase. That's why I wanted to put this on your radar.

[dcaballe]

Just wondering how this will impact the LLVM lowering customization scalability problem in general. By removing the hook for patterns, won't we go from an explosion of populate* functions to an explosion of LLVM lowering passes?

[flaub]

I'm wondering how it will be possible to extend the list of patterns for non-standard ops and still make use of the existing LLVMLoweringPass? I'm currently using the patternListFiller to add my own patterns to the pass. If the LLVMLoweringPass was exposed in a header, I could at least reuse that.

[flaub]

Note: the issue I'm running into is being able to write lit tests that uses the LLVMLoweringPass along with some extra custom patterns. I was able to do that with this little bit of code:

static PassRegistration<Pass> pass("convert-stdx-to-llvm", "Convert stdx to the LLVM dialect", []() {
  return createLowerToLLVMPass([](auto& converter, auto& patterns) {
    populateStdToLLVMConversionPatterns(converter, patterns);
    populateStdXToLLVMConversionPatterns(converter, patterns);
  });
});

[ftynse]

In D73795#1852802, @dcaballe wrote:

Just wondering how this will impact the LLVM lowering customization scalability problem in general. By removing the hook for patterns, won't we go from an explosion of populate* functions to an explosion of LLVM lowering passes?

This looks orthogonal to me. This will not prevent the potential explosion of populate* functions, neither will the presence of the hooks since you'd need to create one composed "populate" function anyway.

[ftynse]

I'm wondering how it will be possible to extend the list of patterns for non-standard ops and still make use of the existing LLVMLoweringPass? I'm currently using the patternListFiller to add my own patterns to the pass. If the LLVMLoweringPass was exposed in a header, I could at least reuse that.

We are talking about ~20 lines of boilerplate vs conceptual complexity of having extension hooks + depending on the decisions std-to-llvm conversion takes to create the pass... Practically, your pass currently inherits a bunch of command-line flags that are specific to std-to-llvm, and it should not. On the higher level, we could expose an LLVMConversionPassBase somewhere, or expose individual patterns like Diego suggested. We might go all the way up to having a ConversionPassBase.

[ftynse]

Rebase

[merge_guards_bot]

Unit tests: pass. 62414 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h

 //===- ConvertStandardToLLVMPass.h - Pass entrypoint ------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_CONVERSION_STANDARDTOLLVM_CONVERTSTANDARDTOLLVMPASS_H_
 #define MLIR_CONVERSION_STANDARDTOLLVM_CONVERTSTANDARDTOLLVMPASS_H_
 
-#include "llvm/ADT/STLExtras.h"
 #include <memory>
-#include <vector>
-
-namespace llvm {
-class Module;
-} // namespace llvm
 
 namespace mlir {
-class DialectConversion;
-class FuncOp;
 class LLVMTypeConverter;
-struct LogicalResult;
-class MLIRContext;
 class ModuleOp;
 template <typename T> class OpPassBase;
-class RewritePattern;
-class Type;
-
-// Owning list of rewriting patterns.
 class OwningRewritePatternList;
 
-/// Type for a callback constructing the owning list of patterns for the
-/// conversion to the LLVMIR dialect.  The callback is expected to append
-/// patterns to the owning list provided as the second argument.
-using LLVMPatternListFiller =
-    std::function<void(LLVMTypeConverter &, OwningRewritePatternList &)>;
-
-/// Type for a callback constructing the type converter for the conversion to
-/// the LLVMIR dialect.  The callback is expected to return an instance of the
-/// converter.
-using LLVMTypeConverterMaker =
-    std::function<std::unique_ptr<LLVMTypeConverter>(MLIRContext *)>;
-
 /// Collect a set of patterns to convert memory-related operations from the
 /// Standard dialect to the LLVM dialect, excluding non-memory-related
 /// operations and FuncOp.
 void populateStdToLLVMMemoryConversionPatters(
     LLVMTypeConverter &converter, OwningRewritePatternList &patterns);
 
 /// Collect a set of patterns to convert from the Standard dialect to the LLVM
 /// dialect, excluding the memory-related operations.

 
 /// Creates a pass to convert the Standard dialect into the LLVMIR dialect.
 /// By default stdlib malloc/free are used for allocating MemRef payloads.
 /// Specifying `useAlloca-true` emits stack allocations instead. In the future
 /// this may become an enum when we have concrete uses for other options.
 std::unique_ptr<OpPassBase<ModuleOp>>
 createLowerToLLVMPass(bool useAlloca = false);
 
-/// Creates a pass to convert operations to the LLVMIR dialect.  The conversion
-/// is defined by a list of patterns and a type converter that will be obtained
-/// during the pass using the provided callbacks.
-/// By default stdlib malloc/free are used for allocating MemRef payloads.
-/// Specifying `useAlloca-true` emits stack allocations instead. In the future
-/// this may become an enum when we have concrete uses for other options.
-std::unique_ptr<OpPassBase<ModuleOp>>
-createLowerToLLVMPass(LLVMPatternListFiller patternListFiller,
-                      LLVMTypeConverterMaker typeConverterMaker,
-                      bool useAlloca = false);
-
-/// Creates a pass to convert operations to the LLVMIR dialect.  The conversion
-/// is defined by a list of patterns obtained during the pass using the provided
-/// callback and an optional type conversion class, an instance is created
-/// during the pass.
-/// By default stdlib malloc/free are used for allocating MemRef payloads.
-/// Specifying `useAlloca-true` emits stack allocations instead. In the future
-/// this may become an enum when we have concrete uses for other options.
-template <typename TypeConverter = LLVMTypeConverter>
-std::unique_ptr<OpPassBase<ModuleOp>>
-createLowerToLLVMPass(LLVMPatternListFiller patternListFiller,
-                      bool useAlloca = false) {
-  return createLowerToLLVMPass(
-      patternListFiller,
-      [](MLIRContext *context) {
-        return std::make_unique<TypeConverter>(context);
-      },
-      useAlloca);
-}
-
 namespace LLVM {
 /// Make argument-taking successors of each block distinct.  PHI nodes in LLVM
 /// IR use the predecessor ID to identify which value to take.  They do not
 /// support different values coming from the same predecessor.  If a block has
 /// another block as a successor more than once with different values, insert
 /// a new dummy block for LLVM PHI nodes to tell the sources apart.
 void ensureDistinctSuccessors(ModuleOp m);
 } // namespace LLVM
 
 } // namespace mlir
 
 #endif // MLIR_CONVERSION_STANDARDTOLLVM_CONVERTSTANDARDTOLLVMPASS_H_

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp

if (!operand.getType().isa<MemRefType>() &&
       continue;
     }
     promotedOperands.push_back(
         promoteOneMemRefDescriptor(loc, llvmOperand, builder));
   }
   return promotedOperands;
 }
 
-/// Create an instance of LLVMTypeConverter in the given context.
-static std::unique_ptr<LLVMTypeConverter>
-makeStandardToLLVMTypeConverter(MLIRContext *context) {
-  LLVMTypeConverterCustomization customs;
-  customs.funcArgConverter = structFuncArgTypeConverter;
-  return std::make_unique<LLVMTypeConverter>(context, customs);
-}
-
-/// Create an instance of BarePtrTypeConverter in the given context.
-static std::unique_ptr<LLVMTypeConverter>
-makeStandardToLLVMBarePtrTypeConverter(MLIRContext *context) {
-  LLVMTypeConverterCustomization customs;
-  customs.funcArgConverter = barePtrFuncArgTypeConverter;
-  return std::make_unique<LLVMTypeConverter>(context, customs);
-}
-
 namespace {
 /// A pass converting MLIR operations into the LLVM IR dialect.
 struct LLVMLoweringPass : public ModulePass<LLVMLoweringPass> {
-  // By default, the patterns are those converting Standard operations to the
-  // LLVMIR dialect.
-  explicit LLVMLoweringPass(
-      bool useAlloca = false,
-      LLVMPatternListFiller patternListFiller =
-          populateStdToLLVMConversionPatterns,
-      LLVMTypeConverterMaker converterBuilder = makeStandardToLLVMTypeConverter)
-      : patternListFiller(patternListFiller),
-        typeConverterMaker(converterBuilder) {}
+  /// Creates an LLVM lowering pass.
+  explicit LLVMLoweringPass(bool useAlloca = false,
+                            bool useBarePtrCallConv = false)
+      : useBarePtrCallConv(useBarePtrCallConv) {}
 
-  // Run the dialect converter on the module.
+  /// Run the dialect converter on the module.
   void runOnModule() override {
-    if (!typeConverterMaker || !patternListFiller)
-      return signalPassFailure();
-
     ModuleOp m = getModule();
     LLVM::ensureDistinctSuccessors(m);
-    std::unique_ptr<LLVMTypeConverter> typeConverter =
-        typeConverterMaker(&getContext());
-    if (!typeConverter)
-      return signalPassFailure();
+
+    LLVMTypeConverterCustomization customs;
+    customs.funcArgConverter = useBarePtrCallConv ? barePtrFuncArgTypeConverter
+                                                  : structFuncArgTypeConverter;
+    LLVMTypeConverter typeConverter(&getContext(), customs);
 
     OwningRewritePatternList patterns;
-    patternListFiller(*typeConverter, patterns);
+    if (useBarePtrCallConv)
+      populateStdToLLVMBarePtrConversionPatterns(typeConverter, patterns);
+    else
+      populateStdToLLVMConversionPatterns(typeConverter, patterns);
 
     ConversionTarget target(getContext());
     target.addLegalDialect<LLVM::LLVMDialect>();
-    if (failed(applyPartialConversion(m, target, patterns, &*typeConverter)))
+    if (failed(applyPartialConversion(m, target, patterns, &typeConverter)))
       signalPassFailure();
   }
 
-  // Callback for creating a list of patterns.  It is called every time in
-  // runOnModule since applyPartialConversion consumes the list.
-  LLVMPatternListFiller patternListFiller;
-
-  // Callback for creating an instance of type converter.  The converter
-  // constructor needs an MLIRContext, which is not available until runOnModule.
-  LLVMTypeConverterMaker typeConverterMaker;
+  bool useBarePtrCallConv;
 };
 } // end namespace
 
 std::unique_ptr<OpPassBase<ModuleOp>>
 mlir::createLowerToLLVMPass(bool useAlloca) {
   return std::make_unique<LLVMLoweringPass>(useAlloca);
 }
 
-std::unique_ptr<OpPassBase<ModuleOp>>
-mlir::createLowerToLLVMPass(LLVMPatternListFiller patternListFiller,
-                            LLVMTypeConverterMaker typeConverterMaker,
-                            bool useAlloca) {
-  return std::make_unique<LLVMLoweringPass>(useAlloca, patternListFiller,
-                                            typeConverterMaker);
-}
-
 static PassRegistration<LLVMLoweringPass>
     pass("convert-std-to-llvm",
          "Convert scalar and vector operations from the "
          "Standard to the LLVM dialect",
          [] {
            return std::make_unique<LLVMLoweringPass>(
-               clUseAlloca.getValue(),
-               clUseBarePtrCallConv ? populateStdToLLVMBarePtrConversionPatterns
-                                    : populateStdToLLVMConversionPatterns,
-               clUseBarePtrCallConv ? makeStandardToLLVMBarePtrTypeConverter
-                                    : makeStandardToLLVMTypeConverter);
+               clUseAlloca.getValue(), clUseBarePtrCallConv.getValue());
          });