[PATCH] D129297: [LSR] Fix bug - check if loop has preheader before calling isInductionPHI

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:1246-1256
+/// Checks if it is safe to call InductionDescriptor::isInductionPHI for \p Phi,
+/// and returns true if this Phi is an induction phi in the loop. When
+/// isInductionPHI returns true, \p ID will be also be set by isInductionPHI.
+static bool checkIsIndPhi(PHINode *Phi, Loop *L, ScalarEvolution *SE,
+                          InductionDescriptor &ID) {
+  if (!Phi)
+    return false;
----------------
syzaara wrote:
> nickdesaulniers wrote:
> > Regarding the comment, should any of these be sunk into the definition of `InductionDescriptor::isInductionPHI`?
> > 
> > For instance, it seems like `InductionDescriptor::isInductionPHI` probably should check that its pointer parameters are not `nullptr`. Other callers of `InductionDescriptor::isInductionPHI` could probably then remove their `nullptr` checks (if they have any).
> > 
> > Otherwise, if the `!L->getLoopPreheader()` and `Phi->getParent() != L->getHeader()` guards should NOT be sunk into the definition of `InductionDescriptor::isInductionPHI`, the added comment doesn't explain _why_ beyond `if it is safe to call`.  _Why_?
> > 
> > For instance `InductionDescriptor::isFPInductionPHI` has a guard
> > ```
> > if (TheLoop->getHeader() != Phi->getParent())
> >     return false;
> > ```
> > _why_ doesn't `InductionDescriptor::isInductionPHI`? Perhaps it should?
> I think it makes sense that these checks could be sink into isInductionPHI. As you said, isFPInductionPHI already checks for 
> ```
> TheLoop->getHeader() != Phi->getParent()
> ```
> I think this can moved into isInductionPHI. Also since isInductionPHI calls:
> 
> ```
>   Value *StartValue =
>       Phi->getIncomingValueForBlock(AR->getLoop()->getLoopPreheader());
> ```
> It would make sense that we check that the loop has a preheader before passing a possible null value.
> 
> I can open a new PR to address this suggestion.
SGTM; please cc me for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129297