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

[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: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();
 
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73912