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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse marked 5 inline comments as done.
ftynse added inline comments.


================
Comment at: mlir/docs/ConversionToLLVMDialect.md:268
+```mlir
+func @foo(%arg0: memref<?xf32>) -> () {
+  "use"(%arg0) : (memref<?xf32>) -> ()
----------------
ftynse wrote:
> 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.
> Can you add an example for how UnrankedMemRef gets unpacked as well ?

Done.


================
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:
> ftynse wrote:
> > rriddle wrote:
> > > This is relaxed because you are inserting multiple operations to perform the conversion?
> > Exactly.
> We erase use_empty cast operations during applyRewrites. How does this interact with generating multiple operations when casting? (This doesn't have to block this revision, but just curious on your thoughts there)
It erases the last operation, but keeps the rest, which are all equally dead and have no side effects. 

It's a variant of the problem we face a lot in rewrites: do we clean up immediately or do we expect the canonicalizer to clean up later. I don't have a good answer here, but I wouldn't go out of my way for cleaning.

In general, I considered the following:
- since we use DialectConversionRewriter, we can use its undo stack to remove all operations introduced in the cast materialization;
- with multiple operations generated, I'm not convinced that we can rely only on one of them (e.g., the last one) to consider the entire conversion dead; we can have stores and other side-effecting operations;
- the canonicalizer is better aware of deadness, so it sounds reasonable to rely on it instead to remove dead casts; (this is similar to the decision not to implement valuesToRemoveIfDead in replaceAllUsesWith IMO)
- if we want the cleanup, maybe we can try and call into the canonicalization directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74211