[PATCH] D129653: isInductionPHI - Add some safety checks

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1537
 
+  if (!(AR->getLoop()->getLoopPreheader()))
+      return false;
----------------
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.


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

https://reviews.llvm.org/D129653