[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:111-114
+  None,   /* Proceed with loop idiom recognize pass */
+  All,    /* Disabled loop idiom recognize pass*/
+  Memcpy, /* Proceed, but do not convert loop to memcpy*/
+  Memset  /* Proceed, but do not convert loop to memset*/
----------------
dfukalov wrote:
> Please prefer single-line comments in such cases.
Thank you! I will change the style of comments.


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


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:243
   bool runOnLoop(Loop *L, LPPassManager &LPM) override {
+    if (DisableLIRP.getValue() == DisableKind::All)
+      return false;
----------------
dfukalov wrote:
> You can omit .getValue() here and below.
Thank you @dfukalov . I will remove .getValue() .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86262