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

Thu Jan 30 17:28:34 PST 2020

dcaballe added a comment.

Thanks for the feedback, again! I addressed it and replied inline.
This is a summary of the pending items that I would suggest addressing separately:

1. Utility to inject a new operation that takes the region argument, and replace all existing uses of the region argument with the new op (@ftynse).
2. Extensibility aspects of the LLVM type converter (@rriddle, @dcaballe can help if needed).
3. Use proper pass options for LLVMLoweringPass flags (@dcaballe).
4. Extensibility aspects of `populate*` functions for LLVMLoweringPass (@dcaballe will bring this to discourse).
5. Add support for CallOp to the bare pointer calling convention (TBD once #4 is addressed).

What do you think? Are you OK with this, @mehdi_amini?

================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:154

-  // Extract an LLVM IR dialect type.
-  LLVM::LLVMType unwrap(Type type);
+  // Callbacks for customizing the type conversion.
+  LLVMTypeConverterCustomization customizations;
----------------
rriddle wrote:
> These should really be using ///
No strong opinion but just for my understanding... if I remember correctly, `///` is for Doxygen purposes so it's only needed for public interfaces and members (?). That's why `//` is used for private members and .cpp documentation. Isn't that the case? At least that's what I've seen around.

LLVM Coding Standards seem a bit ambiguous here:
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

```
Don’t duplicate the documentation comment in the header file and in the implementation file. Put the documentation comments for public APIs into the header file. Documentation comments for private APIs can go to the implementation file. In any case, implementation files can include additional comments (not necessarily in Doxygen markup) to explain implementation details as needed.
```

================
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);

----------------
ftynse wrote:
> 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?
Unused leftover, sorry.

================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:679
+              rewriter, funcLoc, lowering, memrefType, arg);
+          rewriter.replaceUsesOfWith(placeHolder, desc);
+        }
----------------
ftynse wrote:
> 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.
> Even through the rewriter? (rewriter.erase calls rewriter.replaceOp internally)
Oh, that would have worked for the first version, yes! It should be OK now since we are replacing the placeholder (line 684). Thanks!

>This sounds like this should be possible with the materializeConversion hook. I can take a look into that, but it's better to have this landed first.
It sounds good. Thanks!

================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:48
+static llvm::cl::opt<bool> clUseBarePtrCallConv(
+    PASS_NAME "-use-bare-ptr-memref-call-conv",
+    llvm::cl::desc("Replace FuncOp's MemRef arguments with "
----------------
rriddle wrote:
> It would be much better if we could use proper pass options for these instead:
> 
> https://mlir.llvm.org/docs/WritingAPass/#instance-specific-pass-options
I'm just trying to be consistent with the `-use-alloca` flag. I'll change this in a follow-up commit for both.

================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:65

+// Initialize customization to default callbacks.
+LLVMTypeConverterCustomization::LLVMTypeConverterCustomization() {
----------------
rriddle wrote:
> Class/Function/Variable/etc. top-level comments should all be using ///
Same comment about Doxygen.

================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:2370
+  return std::make_unique<LLVMTypeConverter>(context, customs);
 }

----------------
rriddle wrote:
> mehdi_amini wrote:
> > This API seems to still be an issue for extensibility. If you create the LLVMTypeConverter instead of customizing an existing one, how will this the next extension point be implemented/used?
> +1. One direction that I intend to take TypeConverter is to allow registering additional callbacks for the type conversion similarly to how ConversionTarget is extensible. For these types of things, it is increasingly clear how clunky inheritance style modeling is.
I totally agree with that direction! I couldn't see how to go much further in this commit without introducing significant changes in the conversion infra. I think that should happen in a separate commit. Are you OK with the current approach for this commit, @mehdi_amini? Any specific suggestion for this commit that doesn't require too many infra changes?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802