[PATCH] D73592: [IRCE] Make IRCE a Function pass.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1776
+                     /*PreserveLCSSA=*/false);
+    Changed |= formLCSSARecursively(*L, DT, &LI, &SE);
+  }
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > Should only be called if `simplifyLoop` did something. My suggestion is
> >   if (simplifyLoop ...) {
> >     formLCSSA (if that cannot be achieved by setting flag)
> >     Changed = true;
> >   }
> Actually, I am not even sure that a function pass should commit to preserve LCSSA. It is something that only loop passes use.
I don't think, as a Function pass we need to promise preserving LCSSA. In the LoopPassManager (if this was a Loop pass), the two passes (simplify and lcssa) are run to canonicalize the loop. This is the only purpose of running these passes here.
The same approach is used by other function passes that aim to do loop optimizations.


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1783
     if (!IsSubloop)
-      U.addSiblingLoops(NL);
+      appendLoopsToWorklist(*NL, Worklist);
   };
----------------
jdoerfert wrote:
> I know this is not your code but I fail to see why the `LPMAddNewLoop` is different here from below. If it wasn't we could merge almost all of the `run` code into a common helper.
Agreed.
@mkazantsev: Does it makes send to update both `LPMAddNewLoop` to have the `if (!IsSubloop)` condition (see current diff), or is there a specific reason why they were different?
I can send an NFC merging the two code path after that.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1481
+template void llvm::appendLoopsToWorklist<Loop &>(
+    Loop &Loops, SmallPriorityWorklist<Loop *, 4> &Worklist);
+
----------------
jdoerfert wrote:
> Nit: `Loops` is not the right name anymore.
> We could probably also just call the array ref version with `{&L}`
The ArrayRef version above requires an ArrayRef&, so I'd need to allocate a container at the callsite.
Is there another option I missed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73592