[PATCH] D86262: [LoopIdiomRecognizePass] Options to disable part or the entire Loop Idiom Recognize Pass

[Anh Tuyen Tran via Phabricator via llvm-commits]

anhtuyen added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:117
+
+static cl::opt<DisableKind> DisableLIRP(
+    "disable-" DEBUG_TYPE, cl::ReallyHidden,
----------------
etiotto wrote:
> anhtuyen wrote:
> > rzurob wrote:
> > > Based on this option, if a user doesn't want loops to be converted to memcpy or memset, they should specify `--disable-loop-idiom=all`.  But what if another idiom is added to the LoopIdiomRecognize pass in the future?  `--disable-loop-idiom=all` will presumably disable more than what the user asked for.
> > > 
> > > Would it be possible to handle multiple `--disable-loop-idiom=<something>` options cumulatively?  If not, does it make sense to just provide one option to disable the memcpy+memset transformation (i.e. both or neither) instead of this option with all/none/memcpy/memset fine-grain control?
> > > 
> > You raised a good point, thank you @rzurob. Let me re-think about that. 
> > Based on this option, if a user doesn't want loops to be converted to memcpy or memset, they should specify `--disable-loop-idiom=all`.  But what if another idiom is added to the LoopIdiomRecognize pass in the future?  `--disable-loop-idiom=all` will presumably disable more than what the user asked for.
> > 
> > Would it be possible to handle multiple `--disable-loop-idiom=<something>` options cumulatively?  If not, does it make sense to just provide one option to disable the memcpy+memset transformation (i.e. both or neither) instead of this option with all/none/memcpy/memset fine-grain control?
> > 
> 
> I agree and also think is overkill to provide options to disable only part of the transformation, at least at this point. IMO is sufficient to start with a all or nothing option to disable the entire transformation (of course the transformation should continue to be enabled by default).
Thank you very much Rafik @rzurob and Ettore @etiotto for your opinion. I have switched from **cl::opt** to **cl::bits**, which enabled us to handle multiple //**--disable-loop-idiom=<something>**// options cumulatively. I hope that this approach addressed your concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86262