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

Mon Aug 24 06:49:05 PDT 2020

etiotto added a comment.

In D86262#2231209 <https://reviews.llvm.org/D86262#2231209>, @lebedev.ri wrote:

> I'd like to see a PhaseOrdering test showing the IR that is not optimized due to the DA being unaware about these intrinsics.

================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:117
+
+static cl::opt<DisableKind> DisableLIRP(
+    "disable-" DEBUG_TYPE, cl::ReallyHidden,
----------------
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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86262