[PATCH] D73912: [mlir] Turn flags in ConvertStandardToLLVM into pass flags

Mon Feb 10 20:37:36 PST 2020

mehdi_amini added inline comments.

================
Comment at: mlir/include/mlir/Conversion/StandardToLLVM/ConvertStandardToLLVMPass.h:55
 /// this may become an enum when we have concrete uses for other options.
-std::unique_ptr<OpPassBase<ModuleOp>>
-createLowerToLLVMPass(bool useAlloca = false);
+std::unique_ptr<OpPassBase<ModuleOp>> createLowerToLLVMPass();

----------------
dcaballe wrote:
> mehdi_amini wrote:
> > dcaballe wrote:
> > > dcaballe wrote:
> > > > ftynse wrote:
> > > > > rriddle wrote:
> > > > > > flaub wrote:
> > > > > > > When building a pipeline at runtime (that is, not using the command line parsing), it'd be good to be able to specify options during pass creation. I've noticed that currently, even the `useAlloca` bool wasn't actually being used, only the `clUseAlloca` was used. So this is a step in the right direction (using instance specific options rather than global ones), but I think we still need the ability to pass options to the creation function.
> > > > > > +1
> > > > > > I've noticed that currently, even the useAlloca bool wasn't actually being used
> > > > > 
> > > > > I fixed that in e0ea706a59b9032b7f3590478080adf4f3e1486a.
> > > > > 
> > > > > +1 on being able to specify flags at pass creation time.
> > > > Thanks for bringing this up! I'm actually having second thoughts on this patch in general since I'm hitting a problem. Before this change, I was able to use these flags at runtime by passing the flags directly through my nGraph command line. This was very handy since I could experiment with different options without introducing changes or adding/replicating flags on the nGraph side (this is also true for other LLVM cl flags). However, it seems that moving the flags from global cl options to pass cl options means that we can only use these flags with `mlir-opt` since now they are part of the LLVM lowering pass flag (i.e., **-convert-std-to-llvm=**'use-bare-ptr-memref-call-conv=1'), which only makes sense with `mlir-opt` or similar tools that process pass flags. I haven't found a way to use these flags in the same way as before. Maybe there's something missing on my side. Any thoughts about this?
> > > @ftynse, @rriddle, @mehdi_amini, please, let me know what you think about this when you have a chance so that I can move forward. It seems the general consensus is moving global flags to pass flags that will no longer be available externally. However, what happens with those flags that are being used by external clients such as in this case? Should clients duplicate those flags externally? Won't that be too much duplication?
> > >  It seems the general consensus is moving global flags to pass flags
> > 
> > Yes: you can consider this a hard policy in MLIR from my point of view.
> > 
> > > that will no longer be available externally.
> > 
> > Why wouldn't the option be exposed in the API for creating the pass? Options are really part of the API to m.
> > 
> > 
> > > However, what happens with those flags that are being used by external clients such as in this case? Should clients duplicate those flags externally? Won't that be too much duplication?
> > 
> > I don't know what you mean by "duplication": like every API you're interacting with, you need to pass parameters in. The way you're getting this information in the first place is up to the client, maybe you want to expose this to the user with a command line flag, or maybe you want to select a particular configuration for your target.
> > 
> > Putting options in cl::opt is *exclusively* for debugging, these are just not an API.
> > And we should limit these debug flags as much as possible IMO: there is never a reason to use them directly in a pass (pass options should fit all the cases), which leave them for cases deep in libraries where threaded them up to the APIs would be too cumbersome compared to the intended usage.
> Thanks, Mehdi! Don't get me wrong. I agree with the general direction. However, I think there is a gap in functionality. By duplication I meant clients adding the same equivalent flag to toggle a feature that is not on the client side but on the core MLIR side. As a client, I'd like to have my own set of flags but also to be able to forward flags to my internal components when those flags don't impact my code base (something similar to what Clang does with the `-mllvm` flag). For clients interacting with multiple internal components, having to duplicate all the flags of the internal components can be a big deal. I wonder if we could find a way to explicitly forward or selectively expose pass flags to external clients. In any case, a separate discussion. I'll update this patch shortly. Thanks!
> what Clang does with the -mllvm flag

Note this is purely intended for debugging.

> explicitly forward or selectively expose pass flags to external clients

A problem is collision: we need to handle the fact that two passes would both define a "threshold" option for instance.

You could get around this by having this somehow handled by your pipeline builder, taking something like:

`-mlir-pass-option=pass1{threshold=2},pass2{threshold=1}`

Then you can split the string and associate pass1 with the `{threshold=2}` string and use this to build the pass.
Note that this is still not fully general as you may have a pass multiple times in the pipeline...

In the end there is no silver bullet: LLVM has a convenient mechanism based on globals and global registrations, this is nice for playing but quickly annoying when you have to integrate it as a library in a larger product. It causes issues with reproducibility and has limitation around running the same pass multiple times with different options.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73912