[PATCH] D141109: [LoopReroll] Allow for multiple loop control only induction vars

[KAWASHIMA Takahiro via Phabricator via llvm-commits]

kawashima-fj requested changes to this revision.
kawashima-fj added a comment.
This revision now requires changes to proceed.

@caojoshua Thanks for the fix. Allmost looks good to me. I added some minor comments.



================
Comment at: llvm/lib/Transforms/Scalar/LoopRerollPass.cpp:199
 
-    // For loop with multiple induction variable, remember the one used only to
+    // For loop with multiple induction variables, remember the ones used only
     // control the loop.
----------------
`to` is dropped. Is it intentional?


================
Comment at: llvm/lib/Transforms/Scalar/LoopRerollPass.cpp:390
                      DenseMap<Instruction *, int64_t> &IncrMap,
-                     Instruction *LoopCtrlIV)
+                     TinyInstructionVector LoopCtrlIV)
           : Parent(Parent), L(L), SE(SE), AA(AA), TLI(TLI), DT(DT), LI(LI),
----------------
It is better to rename the parameter to `LoopCtrlIVs` (add `s`).


================
Comment at: llvm/lib/Transforms/Scalar/LoopRerollPass.cpp:1184
+  for (Instruction *LoopControlIV : LoopControlIVs) {
+    if (LoopControlIV && LoopControlIV != IV) {
+      for (auto *U : LoopControlIV->users()) {
----------------
This `if` statement may be able to be removed.

- `LoopControlIV` is always non-null.
- `IV` is one of `PossibleIVs`. And in the `collectPossibleIVs` function, a `IV` never goes to both `LoopControlIVs` and `PossibleIVs`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141109