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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/docs/ConversionToLLVMDialect.md:268
+```mlir
+func @foo(%arg0: memref<?xf32>) -> () {
+  "use"(%arg0) : (memref<?xf32>) -> ()
----------------
nmostafa wrote:
> Can you add an example for how UnrankedMemRef gets unpacked as well ? 
> 
> Also, IIUC, if we are passing UnrankedMemRefDescriptor arguments in a loop, we still might exhaust the stack, since we still alloca, copy the MemRefDescriptor and pack the rank and alloca ptr into the UnrankedMemRef struct. Correct ?
Will do.

Yes, unranked memref still allocates once (instead of twice). I don't see an easy way around because it requires a pointer in order to erase the actual type.  There are two things we can explore: outlining the `memref_cast` from ranked to unranked to scope the allocation, using vararg functions and argument reordering.


================
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())))
----------------
rriddle wrote:
> I thought std.varargs was removed? Also, you can use `op->getDialectAttrs()` to filter out the ones without a prefix.
It was still there in the code, so I decided not to touch it in this patch. Will take a look in a follow-up.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:858
+      arg = wrapperArgsRange[0];
+    }
+
----------------
dcaballe wrote:
> drop {}
I prefer not to becase `if` has braces.


================
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));
----------------
rriddle wrote:
> This is relaxed because you are inserting multiple operations to perform the conversion?
Exactly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74211