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

Wed Jan 22 19:57:14 PST 2020

dcaballe marked 2 inline comments as done.
dcaballe added a comment.

Thanks for the feedback, Mehdi!

The reason for having this upstream is because it's generic enough and there were more people interested in lowering MemRefs to bare pointers in LLVM. It's also useful to temporarily overcome the aliasing issue for function arguments, currently impacting everybody using the default LLVM lowering.
However, if you think this is too problematic I can keep this local to our nGraph repository. I would still need some minor changes upstream to enable the overloading of some methods so that we don't have to duplicate and maintain a lot of code.

Thanks,
Diego

================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVM.h:147
+  mlir::Type convertMemRefTypeToBarePtr(mlir::MemRefType type);
 };

----------------
mehdi_amini wrote:
> I am missing how this gets used (other than in testing with the cl::opt) right now?
That's a good point. I'm currently using it locally by invoking `createLowerToLLVM` with the provided `populateStdToLLVMBarePtrConversionPatterns`
`makeStandardToLLVMBarePtrTypeConverter`. I can add changes and a flag to JitRunner so that we can use it through cpu runner. What do you think?

================
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:
> > > 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?

================
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:
> > 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72802