[PATCH] D108114: [LoopPeel] Peel if it turns invariant loads dereferenceable.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

(just submitting responses to comments I forgot to send yesterday)



================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:164
+// Try to find any invariant memory reads that will become dereferenceable in
+// the remainder loop after peeling.
+static unsigned peelToTurnInvariantLoadsDerefencebale(Loop &L,
----------------
mkazantsev wrote:
> Returns... ? Is that supposed to be boolean?
It returns the number of iterations to peel off (added a comment). At the moment it's either 0 or 1, and the main reason it is not a bool is to assign the result directly to `DesiredPeelCount`.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:176
+  assert(L.isLoopExiting(Latch) &&
+         "only multi-exit loops where the latch exits are supported for now");
+
----------------
mkazantsev wrote:
> Can it be a single-exit loop where latch doesn't exit?
Yes, the latch can also be non-exiting. Dropped the assert.


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:200
+      if (auto *LI = dyn_cast<LoadInst>(&I)) {
+        if (DT.dominates(BB, Latch) &&
+            SE.isLoopInvariant(SE.getSCEV(LI->getPointerOperand()), &L))
----------------
reames wrote:
> You don't appear to be requiring the load controls an exit.  It's much less obvious that "just" removing a load is worthwhile.  Consider a huge loop with one potentially invariant load.  
Thanks for the suggestion! I updated the code to track which values/instructions are users of such loads. Peeling is only limited to cases where an exit condition uses such a load (transitively).


================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:201
+        if (DT.dominates(BB, Latch) &&
+            SE.isLoopInvariant(SE.getSCEV(LI->getPointerOperand()), &L))
+          ReadTurnsDeref = true;
----------------
mkazantsev wrote:
> Does this actually lead to the effect you are aiming? Some complexly computed pointer (e.g. result of chain of geps) may be proven loop-invariant by SCEV, but how other passes will figure out this load is from a dereferenceable pointer?
Good point, I changed it to use the weaker `Loop::isLoopInvariant`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108114