Skip to content

Commit

Permalink
Revert "[LV] Unconditionally branch from middle to scalar preheader i…
Browse files Browse the repository at this point in the history 
…f the scalar loop must execute"

This reverts commit 3e5ce49.

Tests started failing on PPC, for example:
http://lab.llvm.org:8011/#/builders/105/builds/5569
  • Loading branch information
@akuegel
akuegel committed Feb 5, 2021
1 parent 78935ea commit 7fe41ac
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 123 deletions.
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/Utils/LoopVersioning.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ LoopVersioning::LoopVersioning(const LoopAccessInfo &LAI,
AliasChecks(Checks.begin(), Checks.end()),
Preds(LAI.getPSE().getUnionPredicate()), LAI(LAI), LI(LI), DT(DT),
SE(SE) {
assert(L->getUniqueExitBlock() && "No single exit block");
}

void LoopVersioning::versionLoop(
const SmallVectorImpl<Instruction *> &DefsUsedOutside) {
assert(VersionedLoop->getUniqueExitBlock() && "No single exit block");
assert(VersionedLoop->isLoopSimplifyForm() &&
"Loop is not in loop-simplify form");

Expand Down
129 changes: 42 additions & 87 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,7 @@ class InnerLoopVectorizer {
/// Middle Block between the vector and the scalar.
BasicBlock *LoopMiddleBlock;

/// The unique ExitBlock of the scalar loop if one exists. Note that
/// The (unique) ExitBlock of the scalar loop. Note that
/// there can be multiple exiting edges reaching this block.
BasicBlock *LoopExitBlock;

Expand Down Expand Up @@ -3147,13 +3147,9 @@ void InnerLoopVectorizer::emitMinimumIterationCountCheck(Loop *L,
DT->getNode(Bypass)->getIDom()) &&
"TC check is expected to dominate Bypass");

// Update dominator for Bypass & LoopExit (if needed).
// Update dominator for Bypass & LoopExit.
DT->changeImmediateDominator(Bypass, TCCheckBlock);
if (!Cost->requiresScalarEpilogue())
// If there is an epilogue which must run, there's no edge from the
// middle block to exit blocks and thus no need to update the immediate
// dominator of the exit blocks.
DT->changeImmediateDominator(LoopExitBlock, TCCheckBlock);
DT->changeImmediateDominator(LoopExitBlock, TCCheckBlock);

ReplaceInstWithInst(
TCCheckBlock->getTerminator(),
Expand Down Expand Up @@ -3192,11 +3188,7 @@ void InnerLoopVectorizer::emitSCEVChecks(Loop *L, BasicBlock *Bypass) {
// Update dominator only if this is first RT check.
if (LoopBypassBlocks.empty()) {
DT->changeImmediateDominator(Bypass, SCEVCheckBlock);
if (!Cost->requiresScalarEpilogue())
// If there is an epilogue which must run, there's no edge from the
// middle block to exit blocks and thus no need to update the immediate
// dominator of the exit blocks.
DT->changeImmediateDominator(LoopExitBlock, SCEVCheckBlock);
DT->changeImmediateDominator(LoopExitBlock, SCEVCheckBlock);
}

ReplaceInstWithInst(
Expand Down Expand Up @@ -3252,11 +3244,7 @@ void InnerLoopVectorizer::emitMemRuntimeChecks(Loop *L, BasicBlock *Bypass) {
// Update dominator only if this is first RT check.
if (LoopBypassBlocks.empty()) {
DT->changeImmediateDominator(Bypass, MemCheckBlock);
if (!Cost->requiresScalarEpilogue())
// If there is an epilogue which must run, there's no edge from the
// middle block to exit blocks and thus no need to update the immediate
// dominator of the exit blocks.
DT->changeImmediateDominator(LoopExitBlock, MemCheckBlock);
DT->changeImmediateDominator(LoopExitBlock, MemCheckBlock);
}

Instruction *FirstCheckInst;
Expand Down Expand Up @@ -3381,10 +3369,9 @@ Value *InnerLoopVectorizer::emitTransformedIndex(
Loop *InnerLoopVectorizer::createVectorLoopSkeleton(StringRef Prefix) {
LoopScalarBody = OrigLoop->getHeader();
LoopVectorPreHeader = OrigLoop->getLoopPreheader();
LoopExitBlock = OrigLoop->getUniqueExitBlock();
assert(LoopExitBlock && "Must have an exit block");
assert(LoopVectorPreHeader && "Invalid loop structure");
LoopExitBlock = OrigLoop->getUniqueExitBlock(); // may be nullptr
assert((LoopExitBlock || Cost->requiresScalarEpilogue()) &&
"multiple exit loop without required epilogue?");

LoopMiddleBlock =
SplitBlock(LoopVectorPreHeader, LoopVectorPreHeader->getTerminator(), DT,
Expand All @@ -3393,20 +3380,12 @@ Loop *InnerLoopVectorizer::createVectorLoopSkeleton(StringRef Prefix) {
SplitBlock(LoopMiddleBlock, LoopMiddleBlock->getTerminator(), DT, LI,
nullptr, Twine(Prefix) + "scalar.ph");

// Set up branch from middle block to the exit and scalar preheader blocks.
// completeLoopSkeleton will update the condition to use an iteration check,
// if required to decide whether to execute the remainder.
BranchInst *BrInst =
BranchInst::Create(LoopExitBlock, LoopScalarPreHeader, Builder.getTrue());
auto *ScalarLatchTerm = OrigLoop->getLoopLatch()->getTerminator();

// Set up the middle block terminator. Two cases:
// 1) If we know that we must execute the scalar epilogue, emit an
// unconditional branch.
// 2) Otherwise, we must have a single unique exit block (due to how we
// implement the multiple exit case). In this case, set up a conditonal
// branch from the middle block to the loop scalar preheader, and the
// exit block. completeLoopSkeleton will update the condition to use an
// iteration check, if required to decide whether to execute the remainder.
BranchInst *BrInst = Cost->requiresScalarEpilogue() ?
BranchInst::Create(LoopScalarPreHeader) :
BranchInst::Create(LoopExitBlock, LoopScalarPreHeader,
Builder.getTrue());
BrInst->setDebugLoc(ScalarLatchTerm->getDebugLoc());
ReplaceInstWithInst(LoopMiddleBlock->getTerminator(), BrInst);

Expand All @@ -3418,11 +3397,7 @@ Loop *InnerLoopVectorizer::createVectorLoopSkeleton(StringRef Prefix) {
nullptr, nullptr, Twine(Prefix) + "vector.body");

// Update dominator for loop exit.
if (!Cost->requiresScalarEpilogue())
// If there is an epilogue which must run, there's no edge from the
// middle block to exit blocks and thus no need to update the immediate
// dominator of the exit blocks.
DT->changeImmediateDominator(LoopExitBlock, LoopMiddleBlock);
DT->changeImmediateDominator(LoopExitBlock, LoopMiddleBlock);

// Create and register the new vector loop.
Loop *Lp = LI->AllocateLoop();
Expand Down Expand Up @@ -3519,14 +3494,10 @@ BasicBlock *InnerLoopVectorizer::completeLoopSkeleton(Loop *L,
auto *ScalarLatchTerm = OrigLoop->getLoopLatch()->getTerminator();

// Add a check in the middle block to see if we have completed
// all of the iterations in the first vector loop. Three cases:
// 1) If we require a scalar epilogue, there is no conditional branch as
// we unconditionally branch to the scalar preheader. Do nothing.
// 2) If (N - N%VF) == N, then we *don't* need to run the remainder.
// Thus if tail is to be folded, we know we don't need to run the
// remainder and we can use the previous value for the condition (true).
// 3) Otherwise, construct a runtime check.
if (!Cost->requiresScalarEpilogue() && !Cost->foldTailByMasking()) {
// all of the iterations in the first vector loop.
// If (N - N%VF) == N, then we *don't* need to run the remainder.
// If tail is to be folded, we know we don't need to run the remainder.
if (!Cost->foldTailByMasking()) {
Instruction *CmpN = CmpInst::Create(Instruction::ICmp, CmpInst::ICMP_EQ,
Count, VectorTripCount, "cmp.n",
LoopMiddleBlock->getTerminator());
Expand Down Expand Up @@ -3590,17 +3561,17 @@ BasicBlock *InnerLoopVectorizer::createVectorizedLoopSkeleton() {
| [ ]_| <-- vector loop.
| |
| v
\ -[ ] <--- middle-block.
\/ |
/\ v
| ->[ ] <--- new preheader.
| -[ ] <--- middle-block.
| / |
| / v
-|- >[ ] <--- new preheader.
| |
(opt) v <-- edge from middle to exit iff epilogue is not required.
| v
| [ ] \
| [ ]_| <-- old scalar loop to handle remainder (scalar epilogue).
| [ ]_| <-- old scalar loop to handle remainder.
\ |
\ v
>[ ] <-- exit block(s).
>[ ] <-- exit block.
...
*/

Expand Down Expand Up @@ -4021,18 +3992,13 @@ void InnerLoopVectorizer::fixVectorizedLoop() {
// Forget the original basic block.
PSE.getSE()->forgetLoop(OrigLoop);

// If we inserted an edge from the middle block to the unique exit block,
// update uses outside the loop (phis) to account for the newly inserted
// edge.
if (!Cost->requiresScalarEpilogue()) {
// Fix-up external users of the induction variables.
for (auto &Entry : Legal->getInductionVars())
fixupIVUsers(Entry.first, Entry.second,
getOrCreateVectorTripCount(LI->getLoopFor(LoopVectorBody)),
IVEndValues[Entry.first], LoopMiddleBlock);
// Fix-up external users of the induction variables.
for (auto &Entry : Legal->getInductionVars())
fixupIVUsers(Entry.first, Entry.second,
getOrCreateVectorTripCount(LI->getLoopFor(LoopVectorBody)),
IVEndValues[Entry.first], LoopMiddleBlock);

fixLCSSAPHIs();
}
fixLCSSAPHIs();
for (Instruction *PI : PredicatedInstructions)
sinkScalarOperands(&*PI);

Expand Down Expand Up @@ -4250,13 +4216,12 @@ void InnerLoopVectorizer::fixFirstOrderRecurrence(PHINode *Phi) {
// recurrence in the exit block, and then add an edge for the middle block.
// Note that LCSSA does not imply single entry when the original scalar loop
// had multiple exiting edges (as we always run the last iteration in the
// scalar epilogue); in that case, there is no edge from middle to exit and
// and thus no phis which needed updated.
if (!Cost->requiresScalarEpilogue())
for (PHINode &LCSSAPhi : LoopExitBlock->phis())
if (any_of(LCSSAPhi.incoming_values(),
[Phi](Value *V) { return V == Phi; }))
LCSSAPhi.addIncoming(ExtractForPhiUsedOutsideLoop, LoopMiddleBlock);
// scalar epilogue); in that case, the exiting path through middle will be
// dynamically dead and the value picked for the phi doesn't matter.
for (PHINode &LCSSAPhi : LoopExitBlock->phis())
if (any_of(LCSSAPhi.incoming_values(),
[Phi](Value *V) { return V == Phi; }))
LCSSAPhi.addIncoming(ExtractForPhiUsedOutsideLoop, LoopMiddleBlock);
}

void InnerLoopVectorizer::fixReduction(PHINode *Phi) {
Expand Down Expand Up @@ -4421,11 +4386,10 @@ void InnerLoopVectorizer::fixReduction(PHINode *Phi) {
// We know that the loop is in LCSSA form. We need to update the PHI nodes
// in the exit blocks. See comment on analogous loop in
// fixFirstOrderRecurrence for a more complete explaination of the logic.
if (!Cost->requiresScalarEpilogue())
for (PHINode &LCSSAPhi : LoopExitBlock->phis())
if (any_of(LCSSAPhi.incoming_values(),
[LoopExitInst](Value *V) { return V == LoopExitInst; }))
LCSSAPhi.addIncoming(ReducedPartRdx, LoopMiddleBlock);
for (PHINode &LCSSAPhi : LoopExitBlock->phis())
if (any_of(LCSSAPhi.incoming_values(),
[LoopExitInst](Value *V) { return V == LoopExitInst; }))
LCSSAPhi.addIncoming(ReducedPartRdx, LoopMiddleBlock);

// Fix the scalar loop reduction variable with the incoming reduction sum
// from the vector body and from the backedge value.
Expand Down Expand Up @@ -8074,11 +8038,7 @@ BasicBlock *EpilogueVectorizerMainLoop::emitMinimumIterationCountCheck(

// Update dominator for Bypass & LoopExit.
DT->changeImmediateDominator(Bypass, TCCheckBlock);
if (!Cost->requiresScalarEpilogue())
// For loops with multiple exits, there's no edge from the middle block
// to exit blocks (as the epilogue must run) and thus no need to update
// the immediate dominator of the exit blocks.
DT->changeImmediateDominator(LoopExitBlock, TCCheckBlock);
DT->changeImmediateDominator(LoopExitBlock, TCCheckBlock);

LoopBypassBlocks.push_back(TCCheckBlock);

Expand Down Expand Up @@ -8142,12 +8102,7 @@ EpilogueVectorizerEpilogueLoop::createEpilogueVectorizedLoopSkeleton() {

DT->changeImmediateDominator(LoopScalarPreHeader,
EPI.EpilogueIterationCountCheck);
if (!Cost->requiresScalarEpilogue())
// If there is an epilogue which must run, there's no edge from the
// middle block to exit blocks and thus no need to update the immediate
// dominator of the exit blocks.
DT->changeImmediateDominator(LoopExitBlock,
EPI.EpilogueIterationCountCheck);
DT->changeImmediateDominator(LoopExitBlock, EPI.EpilogueIterationCountCheck);

// Keep track of bypass blocks, as they feed start values to the induction
// phis in the scalar loop preheader.
Expand Down
14 changes: 8 additions & 6 deletions llvm/test/Transforms/LoopVectorize/first-order-recurrence-complex.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,9 +471,10 @@ define i16 @multiple_exit(i16* %p, i32 %n) {
; CHECK-NEXT: [[TMP15:%.*]] = icmp eq i32 [[INDEX_NEXT]], [[N_VEC]]
; CHECK-NEXT: br i1 [[TMP15]], label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], [[LOOP6:!llvm.loop !.*]]
; CHECK: middle.block:
; CHECK-NEXT: [[CMP_N:%.*]] = icmp eq i32 [[TMP2]], [[N_VEC]]
; CHECK-NEXT: [[VECTOR_RECUR_EXTRACT:%.*]] = extractelement <4 x i16> [[WIDE_LOAD]], i32 3
; CHECK-NEXT: [[VECTOR_RECUR_EXTRACT_FOR_PHI:%.*]] = extractelement <4 x i16> [[WIDE_LOAD]], i32 2
; CHECK-NEXT: br label [[SCALAR_PH]]
; CHECK-NEXT: br i1 [[CMP_N]], label [[IF_END:%.*]], label [[SCALAR_PH]]
; CHECK: scalar.ph:
; CHECK-NEXT: [[SCALAR_RECUR_INIT:%.*]] = phi i16 [ 0, [[ENTRY:%.*]] ], [ [[VECTOR_RECUR_EXTRACT]], [[MIDDLE_BLOCK]] ]
; CHECK-NEXT: [[BC_RESUME_VAL:%.*]] = phi i32 [ [[N_VEC]], [[MIDDLE_BLOCK]] ], [ 0, [[ENTRY]] ]
Expand All @@ -485,14 +486,14 @@ define i16 @multiple_exit(i16* %p, i32 %n) {
; CHECK-NEXT: [[B:%.*]] = getelementptr inbounds i16, i16* [[P]], i64 [[IPROM]]
; CHECK-NEXT: [[REC_NEXT]] = load i16, i16* [[B]], align 2
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[I]], [[N]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[IF_END:%.*]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[IF_END]]
; CHECK: for.body:
; CHECK-NEXT: store i16 [[SCALAR_RECUR]], i16* [[B]], align 4
; CHECK-NEXT: [[INC]] = add nsw i32 [[I]], 1
; CHECK-NEXT: [[CMP2:%.*]] = icmp slt i32 [[I]], 2096
; CHECK-NEXT: br i1 [[CMP2]], label [[FOR_COND]], label [[IF_END]], [[LOOP7:!llvm.loop !.*]]
; CHECK: if.end:
; CHECK-NEXT: [[REC_LCSSA:%.*]] = phi i16 [ [[SCALAR_RECUR]], [[FOR_BODY]] ], [ [[SCALAR_RECUR]], [[FOR_COND]] ]
; CHECK-NEXT: [[REC_LCSSA:%.*]] = phi i16 [ [[SCALAR_RECUR]], [[FOR_BODY]] ], [ [[SCALAR_RECUR]], [[FOR_COND]] ], [ [[VECTOR_RECUR_EXTRACT_FOR_PHI]], [[MIDDLE_BLOCK]] ]
; CHECK-NEXT: ret i16 [[REC_LCSSA]]
;
entry:
Expand Down Expand Up @@ -557,9 +558,10 @@ define i16 @multiple_exit2(i16* %p, i32 %n) {
; CHECK-NEXT: [[TMP15:%.*]] = icmp eq i32 [[INDEX_NEXT]], [[N_VEC]]
; CHECK-NEXT: br i1 [[TMP15]], label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], [[LOOP8:!llvm.loop !.*]]
; CHECK: middle.block:
; CHECK-NEXT: [[CMP_N:%.*]] = icmp eq i32 [[TMP2]], [[N_VEC]]
; CHECK-NEXT: [[VECTOR_RECUR_EXTRACT:%.*]] = extractelement <4 x i16> [[WIDE_LOAD]], i32 3
; CHECK-NEXT: [[VECTOR_RECUR_EXTRACT_FOR_PHI:%.*]] = extractelement <4 x i16> [[WIDE_LOAD]], i32 2
; CHECK-NEXT: br label [[SCALAR_PH]]
; CHECK-NEXT: br i1 [[CMP_N]], label [[IF_END:%.*]], label [[SCALAR_PH]]
; CHECK: scalar.ph:
; CHECK-NEXT: [[SCALAR_RECUR_INIT:%.*]] = phi i16 [ 0, [[ENTRY:%.*]] ], [ [[VECTOR_RECUR_EXTRACT]], [[MIDDLE_BLOCK]] ]
; CHECK-NEXT: [[BC_RESUME_VAL:%.*]] = phi i32 [ [[N_VEC]], [[MIDDLE_BLOCK]] ], [ 0, [[ENTRY]] ]
Expand All @@ -571,14 +573,14 @@ define i16 @multiple_exit2(i16* %p, i32 %n) {
; CHECK-NEXT: [[B:%.*]] = getelementptr inbounds i16, i16* [[P]], i64 [[IPROM]]
; CHECK-NEXT: [[REC_NEXT]] = load i16, i16* [[B]], align 2
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i32 [[I]], [[N]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[IF_END:%.*]]
; CHECK-NEXT: br i1 [[CMP]], label [[FOR_BODY]], label [[IF_END]]
; CHECK: for.body:
; CHECK-NEXT: store i16 [[SCALAR_RECUR]], i16* [[B]], align 4
; CHECK-NEXT: [[INC]] = add nsw i32 [[I]], 1
; CHECK-NEXT: [[CMP2:%.*]] = icmp slt i32 [[I]], 2096
; CHECK-NEXT: br i1 [[CMP2]], label [[FOR_COND]], label [[IF_END]], [[LOOP9:!llvm.loop !.*]]
; CHECK: if.end:
; CHECK-NEXT: [[REC_LCSSA:%.*]] = phi i16 [ [[SCALAR_RECUR]], [[FOR_COND]] ], [ 10, [[FOR_BODY]] ]
; CHECK-NEXT: [[REC_LCSSA:%.*]] = phi i16 [ [[SCALAR_RECUR]], [[FOR_COND]] ], [ 10, [[FOR_BODY]] ], [ [[VECTOR_RECUR_EXTRACT_FOR_PHI]], [[MIDDLE_BLOCK]] ]
; CHECK-NEXT: ret i16 [[REC_LCSSA]]
;
entry:
Expand Down
Loading

0 comments on commit 7fe41ac

Please sign in to comment.