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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Please mark comments as "done" in phab once they're addressed.



================
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;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129297