[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;
----------------
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.


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

https://reviews.llvm.org/D129653