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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe marked an inline comment as done.
dcaballe added inline comments.


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802