This is an archive of the discontinued LLVM Phabricator instance.

[LSR] Fix bug - check if loop has preheader before calling isInductionPHI
ClosedPublic

Authored by syzaara on Jul 7 2022, 8:37 AM.

Details

Summary

Fix bug exposed by https://reviews.llvm.org/D125990, rewriteLoopExitValues calls InductionDescriptor::isInductionPHI which requires the PHI node to have an incoming edge from the loop preheader. This adds checks before calling InductionDescriptor::isInductionPHI to see that the loop has a preheader. Also did some refactoring.

Diff Detail

Event Timeline

syzaara created this revision.Jul 7 2022, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2022, 8:37 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
syzaara requested review of this revision.Jul 7 2022, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2022, 8:37 AM
Meinersbur accepted this revision.Jul 7 2022, 10:35 AM
Meinersbur added a subscriber: nathanchance.

In the interest to not block @nathanchance with a crashing compiler, I suggest to commit as soon as possible. However, a complete patch would also include a regression test.

llvm/lib/Transforms/Utils/LoopUtils.cpp
1249

Please add a doxygen comment briefly describing what this is checking.

This revision is now accepted and ready to land.Jul 7 2022, 10:35 AM

Yes, please add a regression test. Thanks for a quick fix!

syzaara updated this revision to Diff 442998.Jul 7 2022, 11:22 AM

Addressed review comments, added test.

llvm/test/Transforms/LoopStrengthReduce/remove_scev_indvars.ll
97–123

via llvm-reduce, I was able to reduce this test case to:

define void @kernfs_path_from_node() {
entry:
  callbr void asm sideeffect "", "i"(i8* blockaddress(@kernfs_path_from_node, %while.body))
          to label %asm.fallthrough [label %while.body]

asm.fallthrough:                                  ; preds = %entry
  br label %while.body

while.body:                                       ; preds = %while.body, %asm.fallthrough, %entry
  %depth.04 = phi i32 [ %inc, %while.body ], [ 0, %asm.fallthrough ], [ 0, %entry ]
  %inc = add i32 %depth.04, 1
  br i1 false, label %while.end, label %while.body

while.end:                                        ; preds = %while.body
  %inc.lcssa = phi i32 [ %depth.04, %while.body ]
  store i32 %inc.lcssa, i32* null, align 4
  ret void
}
syzaara updated this revision to Diff 443008.Jul 7 2022, 11:45 AM
llvm/lib/Transforms/Utils/LoopUtils.cpp
1249

This comment is not done yet. Please mark checkIsIndPhi as having static linkage.

1260–1262

could these three statements simply be:

return InductionDescriptor::isInductionPHI(Phi, L, SE, ID));

?

syzaara updated this revision to Diff 443018.Jul 7 2022, 11:57 AM

Address review

This revision was landed with ongoing or failed builds.Jul 7 2022, 12:13 PM
This revision was automatically updated to reflect the committed changes.

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

llvm/lib/Transforms/Utils/LoopUtils.cpp
1249–1259

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?

syzaara added inline comments.Jul 7 2022, 12:30 PM
llvm/lib/Transforms/Utils/LoopUtils.cpp
1249–1259

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.

nickdesaulniers added inline comments.Jul 7 2022, 1:16 PM
llvm/lib/Transforms/Utils/LoopUtils.cpp
1249–1259

SGTM; please cc me for review.