[PATCH] D74211: [mlir] use unpacked memref descriptors at function boundaries

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

There is a typo in the description: `porduced`



================
Comment at: mlir/docs/ConversionToLLVMDialect.md:445
+
+Rationale: introducing auxiliary functions for C-compatible interfaces is
+preferred to modifying the calling convention since it will minize the effect
----------------
nit: Introducing


================
Comment at: mlir/docs/ConversionToLLVMDialect.md:446
+Rationale: introducing auxiliary functions for C-compatible interfaces is
+preferred to modifying the calling convention since it will minize the effect
+of C compatibility on intra-module calls or calls between MLIR-generated
----------------
typo: minize -> minimize


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:65
 class LLVMTypeConverter : public TypeConverter {
+  // Give structFuncArgTypeConverter access to memref-specific functions.
+  friend LogicalResult
----------------
nit: ///


================
Comment at: mlir/lib/Conversion/GPUToCUDA/ConvertLaunchFuncToCudaCalls.cpp:243
+//   llvm.store %typeErased, %addr
+//
+// This is necessary to construct the list of arguments passed to the kernel
----------------
nit: /// for the comments.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:199
 
+// In signatures, MemRef descriptors are expanded into lists of non-aggregate
+// values.
----------------
///


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:220
+
+// In signatures, unranked MemRef descriptors are expanded into a pair "rank,
+// pointer to descriptor".
----------------
///


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:743
+        attr.first.is(impl::getTypeAttrName()) ||
+        attr.first.is("std.varargs") ||
+        (filterArgAttrs && impl::isArgAttrName(attr.first.strref())))
----------------
I thought std.varargs was removed? Also, you can use `op->getDialectAttrs()` to filter out the ones without a prefix.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2708
+    if (useBarePtrCallConv && emitCWrappers) {
+      emitError(UnknownLoc::get(&getContext()))
+          << "incompatible conversion options: bare-pointer calling convention "
----------------
getModule().emitError ?


================
Comment at: mlir/lib/Transforms/DialectConversion.cpp:404
         rewriter, origArg.getType(), replArgs, loc);
-    assert(cast->getNumResults() == 1 &&
-           cast->getNumOperands() == replArgs.size());
+    assert(cast->getNumResults() == 1);
     mapping.map(origArg, cast->getResult(0));
----------------
This is relaxed because you are inserting multiple operations to perform the conversion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74211