Skip to content

[SimplifyCFG] Ensure selects have not been constant folded in foldSwitchToSelect #161153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 29, 2025
+40 −22
Merged

[SimplifyCFG] Ensure selects have not been constant folded in foldSwitchToSelect #161153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 21 additions & 22 deletions llvm/lib/Transforms/Utils/SimplifyCFG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@
#include <cstdint>
#include <iterator>
#include <map>
#include <numeric>
#include <optional>
#include <set>
#include <tuple>
Expand Down Expand Up @@ -6356,33 +6355,33 @@ static Value *foldSwitchToSelect(const SwitchCaseResultVectorTy &ResultVector,
if (DefaultResult) {
Value *ValueCompare =
Builder.CreateICmpEQ(Condition, SecondCase, "switch.selectcmp");
SelectInst *SelectValueInst = cast<SelectInst>(Builder.CreateSelect(
ValueCompare, ResultVector[1].first, DefaultResult, "switch.select"));
SelectValue = SelectValueInst;
if (HasBranchWeights) {
SelectValue = Builder.CreateSelect(ValueCompare, ResultVector[1].first,
DefaultResult, "switch.select");
if (auto *SI = dyn_cast<SelectInst>(SelectValue);
SI && HasBranchWeights) {
// We start with 3 probabilities, where the numerator is the
// corresponding BranchWeights[i], and the denominator is the sum over
// BranchWeights. We want the probability and negative probability of
// Condition == SecondCase.
assert(BranchWeights.size() == 3);
setBranchWeights(SelectValueInst, BranchWeights[2],
setBranchWeights(SI, BranchWeights[2],
BranchWeights[0] + BranchWeights[1],
/*IsExpected=*/false);
}
}
Value *ValueCompare =
Builder.CreateICmpEQ(Condition, FirstCase, "switch.selectcmp");
SelectInst *Ret = cast<SelectInst>(Builder.CreateSelect(
ValueCompare, ResultVector[0].first, SelectValue, "switch.select"));
if (HasBranchWeights) {
Value *Ret = Builder.CreateSelect(ValueCompare, ResultVector[0].first,
SelectValue, "switch.select");
if (auto *SI = dyn_cast<SelectInst>(Ret); SI && HasBranchWeights) {
// We may have had a DefaultResult. Base the position of the first and
// second's branch weights accordingly. Also the proability that Condition
// != FirstCase needs to take that into account.
assert(BranchWeights.size() >= 2);
size_t FirstCasePos = (Condition != nullptr);
size_t SecondCasePos = FirstCasePos + 1;
uint32_t DefaultCase = (Condition != nullptr) ? BranchWeights[0] : 0;
setBranchWeights(Ret, BranchWeights[FirstCasePos],
setBranchWeights(SI, BranchWeights[FirstCasePos],
DefaultCase + BranchWeights[SecondCasePos],
/*IsExpected=*/false);
}
Expand Down Expand Up @@ -6422,13 +6421,13 @@ static Value *foldSwitchToSelect(const SwitchCaseResultVectorTy &ResultVector,
Value *And = Builder.CreateAnd(Condition, AndMask);
Value *Cmp = Builder.CreateICmpEQ(
And, Constant::getIntegerValue(And->getType(), AndMask));
SelectInst *Ret = cast<SelectInst>(
Builder.CreateSelect(Cmp, ResultVector[0].first, DefaultResult));
if (HasBranchWeights) {
Value *Ret =
Builder.CreateSelect(Cmp, ResultVector[0].first, DefaultResult);
if (auto *SI = dyn_cast<SelectInst>(Ret); SI && HasBranchWeights) {
// We know there's a Default case. We base the resulting branch
// weights off its probability.
assert(BranchWeights.size() >= 2);
setBranchWeights(Ret, accumulate(drop_begin(BranchWeights), 0),
setBranchWeights(SI, accumulate(drop_begin(BranchWeights), 0),
BranchWeights[0], /*IsExpected=*/false);
}
return Ret;
Expand All @@ -6448,11 +6447,11 @@ static Value *foldSwitchToSelect(const SwitchCaseResultVectorTy &ResultVector,
Value *And = Builder.CreateAnd(Condition, ~BitMask, "switch.and");
Value *Cmp = Builder.CreateICmpEQ(
And, Constant::getNullValue(And->getType()), "switch.selectcmp");
SelectInst *Ret = cast<SelectInst>(
Builder.CreateSelect(Cmp, ResultVector[0].first, DefaultResult));
if (HasBranchWeights) {
Value *Ret =
Builder.CreateSelect(Cmp, ResultVector[0].first, DefaultResult);
if (auto *SI = dyn_cast<SelectInst>(Ret); SI && HasBranchWeights) {
assert(BranchWeights.size() >= 2);
setBranchWeights(Ret, accumulate(drop_begin(BranchWeights), 0),
setBranchWeights(SI, accumulate(drop_begin(BranchWeights), 0),
BranchWeights[0], /*IsExpected=*/false);
}
return Ret;
Expand All @@ -6466,11 +6465,11 @@ static Value *foldSwitchToSelect(const SwitchCaseResultVectorTy &ResultVector,
Value *Cmp2 = Builder.CreateICmpEQ(Condition, CaseValues[1],
"switch.selectcmp.case2");
Value *Cmp = Builder.CreateOr(Cmp1, Cmp2, "switch.selectcmp");
SelectInst *Ret = cast<SelectInst>(
Builder.CreateSelect(Cmp, ResultVector[0].first, DefaultResult));
if (HasBranchWeights) {
Value *Ret =
Builder.CreateSelect(Cmp, ResultVector[0].first, DefaultResult);
if (auto *SI = dyn_cast<SelectInst>(Ret); SI && HasBranchWeights) {
assert(BranchWeights.size() >= 2);
setBranchWeights(Ret, accumulate(drop_begin(BranchWeights), 0),
setBranchWeights(SI, accumulate(drop_begin(BranchWeights), 0),
BranchWeights[0], /*IsExpected=*/false);
}
return Ret;
Expand Down
19 changes: 19 additions & 0 deletions llvm/test/Transforms/SimplifyCFG/switch-to-select-two-case.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,25 @@ bb3:
ret i1 %phi
}

define i32 @negative_constfold_select() {
; CHECK-LABEL: @negative_constfold_select(
; CHECK-NEXT: entry:
; CHECK-NEXT: ret i32 poison
;
entry:
switch i32 poison, label %default [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you adjust this to avoid unconditional UB in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IR before InstCombine: https://llvm.godbolt.org/z/PjdT7fj11. Tried removing immediate UB by still making the Builder return a constant value while handling a constant condition (switch on a previously-assumed equal zero value; store 0 to ptr, load back the value), but having a hard time hitting that code path w/o switching on poison. Maybe I'm missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not easy/possible to make it UB free, then your current test is fine.

i32 0, label %bb
i32 2, label %bb
]

bb:
br label %default

default:
%ret = phi i32 [ poison, %entry ], [ poison, %bb ]
ret i32 %ret
}

!0 = !{!"function_entry_count", i64 1000}
!1 = !{!"branch_weights", i32 3, i32 5, i32 7}
!2 = !{!"branch_weights", i32 3, i32 5, i32 7, i32 11, i32 13}
Expand Down
Loading