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

[Zaara Syeda via Phabricator via llvm-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129297