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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added inline comments.


================
Comment at: mlir/include/mlir/Transforms/DialectConversion.h:324
   /// Replace all the uses of the block argument `from` with value `to`.
-  void replaceUsesOfBlockArgument(BlockArgument from, Value to);
+  void replaceUsesOfWith(Value from, Value to);
 
----------------
rriddle wrote:
> ftynse wrote:
> > dcaballe wrote:
> > > dcaballe wrote:
> > > > rriddle wrote:
> > > > > This function is already broken, I'd rather not expand its scope until its fixed.
> > > > It's currently being used. Why is it broken?
> > > > Maybe I could just replace all the uses (lines 683 and 687) with other utilities but I thought adding a mapping from 'from' to 'to' to pattern-rewriter would be necessary.  
> > > I tried using `replaceAllUsesWith` but it didn't work. The mapping from 'from' to 'to' is needed. I don't see any other option. Any suggestions?
> > @rriddle could you please elaborate on how exactly it is broken?
> Sorry, I sent this when I was OOO and it got lost in my email. The current problem is that the transformation isn't "revertible". If a pattern that uses it would fail, there isn't a guarantee that the IR will be reverted to its original state. It currently internally directly replaces the uses of the argument, which isn't valid given that everything in DialectConversion has to be undoable.
Thanks! Got it now... reverted. I can use `replaceUsesOfBlockArgument` + `rewriter.replaceOp` instead for my use case.


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:679
+              rewriter, funcLoc, lowering, memrefType, arg);
+          rewriter.replaceUsesOfWith(placeHolder, desc);
+        }
----------------
ftynse wrote:
> Should we also erase the placeholder op?
> 
> This sounds there's a missing feature in the rewriter (or the IR in general) to replace all uses except the given ones...
> Should we also erase the placeholder op?
I erased it in my previous version but it was a bug. The placeholder must be alive since it goes into rewriter's mapping.

>This sounds there's a missing feature in the rewriter (or the IR in general) to replace all uses except the given ones...
We would need something like 'domInstFilter' in (https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Transforms/Utils.h#L60) but for the rewritter. I initially considered it but I thought it would be an overkill for this case since we would need to compute dominance information and use `dominates` queries for each use, which is expensive. Also, dominance information would need to be computed at the time of materializing the replacement (?) instead of when adding the replacement to the map, which would make it more complicated. Maybe something to think about separately?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802