[PATCH] D129653: isInductionPHI - Add some safety checks

[Zaara Syeda via Phabricator via llvm-commits]

syzaara marked an inline comment as not done.
syzaara added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1537
 
+  if (!(AR->getLoop()->getLoopPreheader()))
+      return false;
----------------
fhahn wrote:
> nikic wrote:
> > syzaara wrote:
> > > nickdesaulniers wrote:
> > > > syzaara wrote:
> > > > > fhahn wrote:
> > > > > > This is overly restrictive, whether the loop has a pre-header or not shouldn't impact whether a phi is an induction or now.
> > > > > On the line below, we query for AR->getLoop()->getLoopPreheader(). If it turns out that this is null, we will see a crash. I thought adding this would be safer rather than restrictive.
> > > > maybe Phi::getIncomingValueForBlock should return `nullptr` if passed `nullptr`?
> > > Yes, the other option is it move this check into Phi::getIncomingValueForBlock. @fhahn would that be more preferable?
> > It would be possible to relax this to `getLoopPredecessor()`. Without a loop predecessor we would have to guard against multiple starting values, which is probably not worthwhile?
> > If it turns out that this is null, we will see a crash
> 
> Oh yes, this also seems overly restrictive :/
> 
> The main thing we need an IR value to use as start value.
> 
> Given that we already require a single latch, checking that all non-latch incoming values are the same should be fairly straight-forward :) 
> 
> Or maybe it is fine to pick any one of the non-latch incoming values as start value. We require the phi to form an AddRec, so it should have a single start value. Even if the incoming values are different IR values, they should evaluate to the same concrete value.
Ok, I can try picking any of the non-latch incoming values and test that out. Another option is what isFPInductionPHI does:

```
  // The loop may have multiple entrances or multiple exits; we can analyze                 
  // this phi if it has a unique entry value and a unique backedge value.                   
  if (Phi->getNumIncomingValues() != 2)                                                     
    return false;                                                                           
  Value *BEValue = nullptr, *StartValue = nullptr;
  if (TheLoop->contains(Phi->getIncomingBlock(0))) {                                        
    BEValue = Phi->getIncomingValue(0);                                                     
    StartValue = Phi->getIncomingValue(1);                                                  
  } else {
    assert(TheLoop->contains(Phi->getIncomingBlock(1)) &&
           "Unexpected Phi node in the loop");
    BEValue = Phi->getIncomingValue(1);                                                     
    StartValue = Phi->getIncomingValue(0);
  } 
```


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

https://reviews.llvm.org/D129653