[PATCH] D72802: [mlir] Introduce bare ptr calling convention for MemRefs in LLVM dialect

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.

It would be nice to see in a future revision if we can cleanup some of the extensibility aspects of the LLVM type converter.



================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:55
+
+// Callback to convert function argument types. It converts MemRef function
+// arguments to bare pointers to the MemRef element type. Converted types are
----------------
// -> ///


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:66
 
+  /// Create an LLVMTypeConverter using default LLVMTypeConverterCustomization.
   LLVMTypeConverter(MLIRContext *ctx);
----------------
using the


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:154
 
-  // Extract an LLVM IR dialect type.
-  LLVM::LLVMType unwrap(Type type);
+  // Callbacks for customizing the type conversion.
+  LLVMTypeConverterCustomization customizations;
----------------
These should really be using ///


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:48
+static llvm::cl::opt<bool> clUseBarePtrCallConv(
+    PASS_NAME "-use-bare-ptr-memref-call-conv",
+    llvm::cl::desc("Replace FuncOp's MemRef arguments with "
----------------
It would be much better if we could use proper pass options for these instead:

https://mlir.llvm.org/docs/WritingAPass/#instance-specific-pass-options


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:65
 
+// Initialize customization to default callbacks.
+LLVMTypeConverterCustomization::LLVMTypeConverterCustomization() {
----------------
Class/Function/Variable/etc. top-level comments should all be using ///


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:89
+  SmallVector<int64_t, 4> strides;
+  if (!succeeded(getStridesAndOffset(type, strides, offset)))
+    return {};
----------------
failed


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:96
+    return {};
+  auto ptrTy = elementType.getPointerTo(type.getMemorySpace());
+  return ptrTy;
----------------
Why not just return directly?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:659
+    auto newFuncOp = convertFuncOpToLLVMFuncOp(funcOp, rewriter);
+
+    if (newFuncOp.getBody().empty()) {
----------------
nit: Remove the newline


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2370
+  return std::make_unique<LLVMTypeConverter>(context, customs);
 }
 
----------------
mehdi_amini wrote:
> This API seems to still be an issue for extensibility. If you create the LLVMTypeConverter instead of customizing an existing one, how will this the next extension point be implemented/used?
+1. One direction that I intend to take TypeConverter is to allow registering additional callbacks for the type conversion similarly to how ConversionTarget is extensible. For these types of things, it is increasingly clear how clunky inheritance style modeling is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72802/new/

https://reviews.llvm.org/D72802