[PATCH] D129653: isInductionPHI - Add some safety checks

[Zaara Syeda via Phabricator via llvm-commits]

syzaara added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1537
 
+  if (!(AR->getLoop()->getLoopPreheader()))
+      return false;
----------------
fhahn wrote:
> syzaara wrote:
> > 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);
> >   } 
> > ```
> That seems like a good first step. IMO it would also make sense for them both to share the same logic to get the start value.
@fhahn can you please take a look for review? Thank you!


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

https://reviews.llvm.org/D129653