Skip to content

Commit 0f7cc41

Browse files
authored
[CVP] Keep ReachableCaseCount in sync with range of condition (#142302)
#79993 assumes that a reachable case must be contained by `CR`. However, it doesn't hold for some edge cases. This patch adds additional checks to ensure `ReachableCaseCount` is correct. Note: Similar optimization in SCCP isn't affected by this bug because it uses `CR` to compute `ReachableCaseCount`. Closes #142286.
1 parent b9fa1df commit 0f7cc41

File tree

2 files changed

+70
-22
lines changed

2 files changed

+70
-22
lines changed

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -370,15 +370,30 @@ static bool processSwitch(SwitchInst *I, LazyValueInfo *LVI,
370370
{ // Scope for SwitchInstProfUpdateWrapper. It must not live during
371371
// ConstantFoldTerminator() as the underlying SwitchInst can be changed.
372372
SwitchInstProfUpdateWrapper SI(*I);
373+
ConstantRange CR =
374+
LVI->getConstantRangeAtUse(I->getOperandUse(0), /*UndefAllowed=*/false);
373375
unsigned ReachableCaseCount = 0;
374376

375377
for (auto CI = SI->case_begin(), CE = SI->case_end(); CI != CE;) {
376378
ConstantInt *Case = CI->getCaseValue();
377-
auto *Res = dyn_cast_or_null<ConstantInt>(
378-
LVI->getPredicateAt(CmpInst::ICMP_EQ, Cond, Case, I,
379-
/* UseBlockValue */ true));
379+
std::optional<bool> Predicate = std::nullopt;
380+
if (!CR.contains(Case->getValue()))
381+
Predicate = false;
382+
else if (CR.isSingleElement() &&
383+
*CR.getSingleElement() == Case->getValue())
384+
Predicate = true;
385+
if (!Predicate) {
386+
// Handle missing cases, e.g., the range has a hole.
387+
auto *Res = dyn_cast_or_null<ConstantInt>(
388+
LVI->getPredicateAt(CmpInst::ICMP_EQ, Cond, Case, I,
389+
/* UseBlockValue=*/true));
390+
if (Res && Res->isZero())
391+
Predicate = false;
392+
else if (Res && Res->isOne())
393+
Predicate = true;
394+
}
380395

381-
if (Res && Res->isZero()) {
396+
if (Predicate && !*Predicate) {
382397
// This case never fires - remove it.
383398
BasicBlock *Succ = CI->getCaseSuccessor();
384399
Succ->removePredecessor(BB);
@@ -395,7 +410,7 @@ static bool processSwitch(SwitchInst *I, LazyValueInfo *LVI,
395410
DTU.applyUpdatesPermissive({{DominatorTree::Delete, BB, Succ}});
396411
continue;
397412
}
398-
if (Res && Res->isOne()) {
413+
if (Predicate && *Predicate) {
399414
// This case always fires. Arrange for the switch to be turned into an
400415
// unconditional branch by replacing the switch condition with the case
401416
// value.
@@ -410,27 +425,24 @@ static bool processSwitch(SwitchInst *I, LazyValueInfo *LVI,
410425
++ReachableCaseCount;
411426
}
412427

413-
if (ReachableCaseCount > 1 && !SI->defaultDestUnreachable()) {
428+
// The default dest is unreachable if all cases are covered.
429+
if (!SI->defaultDestUnreachable() &&
430+
!CR.isSizeLargerThan(ReachableCaseCount)) {
414431
BasicBlock *DefaultDest = SI->getDefaultDest();
415-
ConstantRange CR = LVI->getConstantRangeAtUse(I->getOperandUse(0),
416-
/*UndefAllowed*/ false);
417-
// The default dest is unreachable if all cases are covered.
418-
if (!CR.isSizeLargerThan(ReachableCaseCount)) {
419-
BasicBlock *NewUnreachableBB =
420-
BasicBlock::Create(BB->getContext(), "default.unreachable",
421-
BB->getParent(), DefaultDest);
422-
new UnreachableInst(BB->getContext(), NewUnreachableBB);
432+
BasicBlock *NewUnreachableBB =
433+
BasicBlock::Create(BB->getContext(), "default.unreachable",
434+
BB->getParent(), DefaultDest);
435+
new UnreachableInst(BB->getContext(), NewUnreachableBB);
423436

424-
DefaultDest->removePredecessor(BB);
425-
SI->setDefaultDest(NewUnreachableBB);
437+
DefaultDest->removePredecessor(BB);
438+
SI->setDefaultDest(NewUnreachableBB);
426439

427-
if (SuccessorsCount[DefaultDest] == 1)
428-
DTU.applyUpdates({{DominatorTree::Delete, BB, DefaultDest}});
429-
DTU.applyUpdates({{DominatorTree::Insert, BB, NewUnreachableBB}});
440+
if (SuccessorsCount[DefaultDest] == 1)
441+
DTU.applyUpdates({{DominatorTree::Delete, BB, DefaultDest}});
442+
DTU.applyUpdates({{DominatorTree::Insert, BB, NewUnreachableBB}});
430443

431-
++NumDeadCases;
432-
Changed = true;
433-
}
444+
++NumDeadCases;
445+
Changed = true;
434446
}
435447
}
436448

llvm/test/Transforms/CorrelatedValuePropagation/switch.ll

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,42 @@ cleanup:
294294
ret i32 %retval.0
295295
}
296296

297+
; Make sure that we don't branch into unreachable.
298+
299+
define void @pr142286() {
300+
; CHECK-LABEL: define void @pr142286() {
301+
; CHECK-NEXT: start:
302+
; CHECK-NEXT: br label [[LOOP:%.*]]
303+
; CHECK: loop:
304+
; CHECK-NEXT: br label [[LOOP2:%.*]]
305+
; CHECK: loop2:
306+
; CHECK-NEXT: br label [[LOOP3:%.*]]
307+
; CHECK: loop3:
308+
; CHECK-NEXT: br label [[EXIT:%.*]]
309+
; CHECK: exit:
310+
; CHECK-NEXT: ret void
311+
;
312+
start:
313+
br label %loop
314+
315+
loop:
316+
%phi = phi i8 [ -1, %start ], [ 0, %loop3 ]
317+
br label %loop2
318+
319+
loop2:
320+
br label %loop3
321+
322+
loop3:
323+
switch i8 %phi, label %exit [
324+
i8 0, label %loop3
325+
i8 1, label %loop2
326+
i8 2, label %loop
327+
]
328+
329+
exit:
330+
ret void
331+
}
332+
297333
declare i32 @call0()
298334
declare i32 @call1()
299335
declare i32 @call2()

0 commit comments

Comments
 (0)