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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:147
+  mlir::Type convertMemRefTypeToBarePtr(mlir::MemRefType type);
 };
 
----------------
I am missing how this gets used (other than in testing with the cl::opt) right now?


================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h:76
+    LLVMTypeConverter &converter, OwningRewritePatternList &patterns);
+
 /// Creates a pass to convert the Standard dialect into the LLVMIR dialect.
----------------
dcaballe wrote:
> mehdi_amini wrote:
> > I'm concerned about the scalability of this approach.
> > 
> > You're making the type converter extensible through inheritance and overloading, which is not gonna compose well: it is impossible to another orthogonal customization point.
> > Then extending it with a new subclass requires new header entry points: the multiple populate* functions does not seems very nice to me already, and this is making it just harder to figure out how to use this header.
> > 
> > If we are convinced that we don't need to scale or that we won't need more customization point, then I rather not use any virtual function and look into passing a config struct with flag on the lowering behavior we want.
> > 
> Thanks for the feedback, Mehdi.
> 
> > You're making the type converter extensible through inheritance and overloading, which is not gonna compose well: it is impossible to another orthogonal customization point.
> 
> I think this looks better after the refactoring since I'm only introducing `convertArgType`, which should be a unitary piece of the type conversion and easy to compose with other customizations. If I understood correctly, the general direction is that developers may want to create custom lowerings to LLVM, even out of tree. If that is the case, we should try to make the LLVM type converter more friendly to that and facilitate code reuse as much as possible. I think that breaking LLVM type conversion into smaller unitary chucks would help and this patch goes towards that direction. If eventually the current LLVMTypeConverter makes composability "impossible to another orthogonal customization points", we could move the current "default" implementation to a sub-class and only keep the code or API that is generic enough, unitary and reusable in LLVMTypeConverter. Would that make sense?
> 
> > Then extending it with a new subclass requires new header entry points: the multiple populate* functions does not seems very nice to me already, and this is making it just harder to figure out how to use this header.
> 
> I see your point. IIRC, the recently added public populate* functions were aimed at facilitating the reuse of existing patterns in custom lowerings, particularly out of tree. Currently, there is no other way to make these patterns available since they are private to this translation unit. If we don't want to change the latter, we could publicly expose only the populate* functions that provide all the conversion patterns (i.e., `populateStdToLLVMConversionPatterns` and `populateStdToLLVMBarePtrConversionPatterns`). Someone could always add custom patterns with more priority to override the default ones. Not a very clean solution, though. 
> 
> > If we are convinced that we don't need to scale or that we won't need more customization point, then I rather not use any virtual function and look into passing a config struct with flag on the lowering behavior we want.
> 
> It's difficult to know at this point and the feedback that I've heard so far is in the direction of having custom lowerings. I suggested an abstraction to customize memref lowering that didn't go through (https://github.com/tensorflow/mlir/pull/337) and more people are interested in lowering memrefs in different ways (https://groups.google.com/a/tensorflow.org/forum/?utm_medium=email&utm_source=footer#!msg/mlir/9UcFIefP9u0/3ujw73F8BAAJ)
> 
> I think this looks better after the refactoring since I'm only introducing convertArgType, which should be a unitary piece of the type conversion and easy to compose with other customizations. 

I'm not sure I understand what you mean here: are you saying that LLVMTypeConverter will never ever have any other virtual method that you would need for customization?

>  I think that breaking LLVM type conversion into smaller unitary chucks would help and this patch goes towards that direction.

How is this patch going in this direction? Maybe I don't perceive the direction you're referring to with "smaller unitary chunks" here?

>  we could publicly expose only the populate* functions that provide all the conversion patterns (i.e., populateStdToLLVMConversionPatterns and populateStdToLLVMBarePtrConversionPatterns). 

Again, I see some composability issue. What about the next orthogonal option? Are we gonna multiply the number of `populateStdToLLVM*ConversionPatterns` function by two? There is a combinatorial aspect that I don't understand how you see solved.

Unless the "BarePtr" is the only ever customization we want to apply, I don't see how this is scaling forward right now.

Have you looked into injection instead of subclassing here?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802