[PATCH] D69830: [LoopPred/WC] Use a dominating widenable condition to remove analyze loop exits

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:965
+/// loop, return it.
+static BranchInst *FindWideableTerminatorAboveLoop(Loop *L, LoopInfo &LI) {
+  // Walk back through any unconditional executed blocks and see if we can find
----------------
typo - WideNable


================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:1127-1128
+    Value *NewCond = B.CreateICmp(ICmpInst::ICMP_UGT, ECV, RHS);
+    // TODO: NewCond = B.CreateFreeze(NewCond) once freeze inst submitted.
+    // See NOTE ON POISON/UNDEF above for context.
+
----------------
I have not been following this 'freeze' story, but freeze instruction is already in:
```
commit 58acbce3def63a207b8f5a69318a99666a4aac53
Author: aqjune <aqjune at gmail.com>
Date:   Tue Nov 5 15:53:22 2019 +0900
    [IR] Add Freeze instruction
```
Perhaps you want to rephrase this TODO ...


================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:1206
   bool Changed = false;
-  for (auto *Guard : Guards)
-    Changed |= widenGuardConditions(Guard, Expander);
-  for (auto *Guard : GuardsAsWidenableBranches)
-    Changed |= widenWidenableBranchGuardConditions(Guard, Expander);
-
+  if (!Guards.empty() || !GuardsAsWidenableBranches.empty()) { 
+    for (auto *Guard : Guards)
----------------
IMO it would look more natural would you remove this if() check, leaving two guard loops on the same level as predicateLoopExits call.
There should be no extra efficiency gained from this (superflous) check.


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

https://reviews.llvm.org/D69830