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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

I'm supportive of this change as long as Mehdi's and River's concerns are addressed.



================
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:
> dcaballe wrote:
> > mehdi_amini wrote:
> > > 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?
> > > 
> > > 
> > >>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?
> > 
> > Not at all! IIUC, your concern about composability was on the first version of the code where I was overloading `convertFunctionSignature`, a conversion method that does quite a few simple steps altogether (convert argument types, convert return type, create a new function type, etc.). I agree that overloading that method would make composability/reuse of those simple steps difficult. In the second version, I refactored the code that converted argument types into an independent function. By refactoring code into smaller virtual functions doing only a simple thing (that's what I meant by unitary), we should be able to provide a higher level of customization and composability by just overloading the small function that refers to the part of the conversion that we want to customize.
> > 
> > Maybe if you could provide an example of an "orthogonal customization" we could discuss based on something specific.
> > 
> > >> 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?
> > 
> > Hopefully it's clearer now with my answer above.
> > 
> > >> 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.
> > 
> > This is a different composability problem but, yes, I agree with you. However, I don't think we are aiming at having every potential combination of customization available. If that were the case, we would have a combinatorial problem regardless of these interfaces being public or not. If we really need a high level of composability here, we could make all the LLVM conversion patterns public so that each client could build its own customized "populate" function. Pinging @ftynse since he also introduced some of these functions.
> > 
> > > Have you looked into injection instead of subclassing here?
> > 
> > I assume this refers to the type conversion again. Not sure how injection could improve the design here. We would need either virtual functions or refactoring common code to utility functions. Do you see something I'm missing? Also, type converter is already injected so I think it would make usability more complicated. What do you think?
> > By refactoring code into smaller virtual functions doing only a simple thing (that's what I meant by unitary), we should be able to provide a higher level of customization and composability by just overloading the small function that refers to the part of the conversion that we want to customize.
> >
> > Maybe if you could provide an example of an "orthogonal customization" we could discuss based on something specific.
> 
> 
> Sure, let me clarify: right now you're overloading `convertArgType` by adding `BarePtrTypeConverter` , which only deals with MemRef function arguments.
> If later we want to have another customization point to handle conversion for other kind of argument, let say composite type. We'd have to overload the same method in another subclass `CustomCompositeTypeConverter`. But how would these two subclasses compose with each other? How do I get both the `bare` memref ABI and the custom composite types?
> 
> Expand this to other customization points: assume we want to control the return type of function. We'll create another virtual function similar to `convertArgType`, let say `convertResultType`.
> We can overload this in subclasses, but again if one subclass is overloading `convertResultType` but I also want the `BarePtrTypeConverter`, how do I proceed?
> 
> > I assume this refers to the type conversion again. Not sure how injection could improve the design here. We would need either virtual functions or refactoring common code to utility functions. Do you see something I'm missing? Also, type converter is already injected so I think it would make usability more complicated. What do you think?
> 
> What I'm referring to is that you instead of virtual functions, you can have a callback or a list of callbacks for each of the possible extension point.
> For instance instead of having: `LLVM::LLVMType convertArgType(Type type);` as a virtual method you could store:
> 
> ```
> std::vector<std::function<LLVM::LLVMType(Type type)>> convertArgTypeImpls;
> ```
> 
> And have:
> 
> ```
> LLVM::LLVMType convertArgType(Type type) {
>   for (auto &callback : convertArgTypeImpls) {
>     type = callback(type);
>     // possibly early exit on the first success instead?
>   }
>   return type;
> }
> ```
> 
> This allows to inject customization both for different types and/or for different customization points (another vector of callbacks for example).
> 
> 
It sounds like the scalable approach would be to implement type conversion following the similar pattern-based architecture we are building for op conversion... I considered doing that at some point.

An intermediate proposition could be to do the dispatch manually instead of relying on virtual functions with something like

```
class LLVMTypeCoverter {
  std::function<LLVMType(Type)> memrefConverter;
  std::function<LLVMType(Type)> funcArgumentConverter;
  // ...
};
```


================
Comment at: mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp:273
+  bool strideSuccess = succeeded(getStridesAndOffset(type, strides, offset));
+  assert(strideSuccess &&
+         "Non-strided layout maps must have been normalized away");
----------------
I'd rather return null here. This can happen in correct code and we shouldn't crash. Normally, the type conversion should propagate null-as-error-mark.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802