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

[Philip Reames via Phabricator via llvm-commits]

reames marked 3 inline comments as done.
reames added a comment.

Patch refresh coming to incorporate comments.

In D69830#1742086 <https://reviews.llvm.org/D69830#1742086>, @ebrevnov wrote:

> In general the code looks fine to me. What is not clear how it is intended to work on real case scenarios. On the one hand we rely on WC having explicit form in the IR (ExitBB->getTerminatingDeoptimizeCall()) on the other hand we skip optimizing exits over WC since they are not analyzable. Are you going to introduce that support in the next patch or I'm missing something here?


I'm confused by your confusion.

This patch widens checks in the loop (not involving widenable conditions) into a check before the loop (involving a widenable condition).  It does not widen widenable conditions *within* a loop.

We will hoist and unswitch branches within a loop based on widenable conditions.  As such, it seems reasonable to believe widenable conditions can be usually found outside of loops.



================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:1108
+    BasicBlock *ExitBB = BI->getSuccessor(ExitIfTrue ? 0 : 1);
+    if (!ExitBB->getTerminatingDeoptimizeCall())
+      // Profitability: indicator of rarely/never taken exit
----------------
fedor.sergeev wrote:
> ebrevnov wrote:
> > fedor.sergeev wrote:
> > > ebrevnov wrote:
> > > > Would it be better to check profitability in a more general way using BPI or similar?
> > > Since widenable branch jumping to deoptimize is the main intended usecase for widenable branch itself
> > > (we even show it in LangRef as a prime example of widenable.condition usage)
> > > it seems to be a very good special case to start with.
> > > 
> > > BPI is tricky to use (and tricky to preserve), so it can not be the main source of truth for profitability purposes.
> > > 
> > First I don't see anything tricky in using BPI. It is widely used across optimizer and if BPI is "incorrect" that may affect lot of places. Moreover LoopPredication has explicit dependence on BPI and won't work as expected with "broken" BPI anway.  Also note that current implementation doesn't handle widenable branches because such branches are not analyzable.
> > LoopPredication has explicit dependence on BPI and won't work as expected with "broken" BPI anway.
> I'm not talking about "broken BPI". 
> In new PassManager BPI is not always preserved through the loop passes, so it can be just "absent".
> 
> And yes, current profitability checks do depend on BPI, yet LoopPredication will just consider loop as unconditionally profitable in absence of BPI.
> 
I would strongly prefer not to introduce BPI here.  Having an deoptimize call is a very strong signal of cold code.  BPI doesn't really have a corresponding notion of "almost definitely cold code".  

(I'm not opposed to trying to generalize, I just don't want to do it now.  There are other parts I consider more important.)


================
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.
+
----------------
fedor.sergeev wrote:
> 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 ...
It hadn't at the point I posted the patch.  I will add support when I refresh patch.  


================
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)
----------------
fedor.sergeev wrote:
> 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.
Hm, you're right.  Will do.


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

https://reviews.llvm.org/D69830