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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

I'm fine with this, but please make sure @rriddle has no objections either.



================
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);
 
----------------
dcaballe wrote:
> 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.
I still see `replaceUsesOfValue` declared below, did you forget to upload something?


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:679
+              rewriter, funcLoc, lowering, memrefType, arg);
+          rewriter.replaceUsesOfWith(placeHolder, desc);
+        }
----------------
dcaballe wrote:
> 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?
> I erased it in my previous version but it was a bug. The placeholder must be alive since it goes into rewriter's mapping.

Even through the rewriter?  (`rewriter.erase` calls `rewriter.replaceOp` internally)

> We would need something like 'domInstFilter' 

It looks like here we are in a more specific case, which seems to come back several times on the function boundary: we need to inject a new operation that takes the region argument, and replace all existing uses of the region argument with the new op. This sounds like this should be possible with the `materializeConversion` hook (https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Transforms/DialectConversion.h#L135).  I can take a look into that, but it's better to have this landed first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802