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

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache accepted this revision.
nicolasvasilache added a comment.

Minor (ultra)-nit.
Looks great to me, this should also help with @dcaballe 's issues with forcing the memref descriptor ABI on everything.

Thanks for this great patch!



================
Comment at: mlir/docs/ConversionToLLVMDialect.md:363
+
+1. Define a new function `_mlir_ciface_<origina name>` where memref arguments
+   are converted to pointer-to-struct and the remaining arguments are converted
----------------
typo original


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:162
+  /// For example, memref<?x?xf32> is converted to the following list:
+  /// !llvm<"float*"> (allocated pointer),
+  /// !llvm<"float*"> (aligned pointer),
----------------
ultra-nit: extra space before each line to make the list pop out better.
Or use a proper list and backticks as such:
- `!llvm...`


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:286
+
+/// Creates descriptor structs from indiivdual values consituting them.
+Operation *LLVMTypeConverter::materializeConversion(PatternRewriter &rewriter,
----------------
typo individual


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:609
+  int64_t rank = type.getRank();
+  results.reserve(results.size() + 2 * rank + 3);
+
----------------
call `getNumUnpackedValues` for a single source of truth.


================
Comment at: mlir/test/Conversion/StandardToLLVM/convert-funcs.mlir:21
 
-// Check that memrefs are converted to pointers-to-struct if appear as function arguments.
-// CHECK: llvm.func @memref_call_conv(!llvm<"{ float*, float*, i64, [1 x i64], [1 x i64] }*">)
+// Check that memrefs are converted to argyment packs if appear as function arguments.
+// CHECK: llvm.func @memref_call_conv(!llvm<"float*">, !llvm<"float*">, !llvm.i64, !llvm.i64, !llvm.i64)
----------------
typo argument


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74211