Skip to content
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

Jump to bottom

Reland [SimplifyCFG] Delete the unnecessary range check for small mask operation #70542

Merged
merged 2 commits into from
Nov 3, 2023
Merged

Reland [SimplifyCFG] Delete the unnecessary range check for small mask operation #70542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e86a303
[SimplifyCFG] Delete the unnecessary range check for small mask opera…
vfdff Sep 2, 2023
64be382
[SimplifyCFG] Fix the compile crash when default branch is unreachable
vfdff Oct 28, 2023
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
24 changes: 22 additions & 2 deletions llvm/lib/Transforms/Utils/SimplifyCFG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6598,9 +6598,8 @@ static bool SwitchToLookupTable(SwitchInst *SI, IRBuilder<> &Builder,
// If the default destination is unreachable, or if the lookup table covers
// all values of the conditional variable, branch directly to the lookup table
// BB. Otherwise, check that the condition is within the case range.
const bool DefaultIsReachable =
bool DefaultIsReachable =
!isa<UnreachableInst>(SI->getDefaultDest()->getFirstNonPHIOrDbg());
const bool GeneratingCoveredLookupTable = (MaxTableSize == TableSize);

// Create the BB that does the lookups.
Module &Mod = *CommonDest->getParent()->getParent();
Expand Down Expand Up @@ -6631,6 +6630,27 @@ static bool SwitchToLookupTable(SwitchInst *SI, IRBuilder<> &Builder,

BranchInst *RangeCheckBranch = nullptr;

// Grow the table to cover all possible index values to avoid the range check.
// It will use the default result to fill in the table hole later, so make
// sure it exist.
if (UseSwitchConditionAsTableIndex && HasDefaultResults) {
vfdff marked this conversation as resolved.
Show resolved Hide resolved
ConstantRange CR = computeConstantRange(TableIndex, /* ForSigned */ false);
// Grow the table shouldn't have any size impact by checking
// WouldFitInRegister.
// TODO: Consider growing the table also when it doesn't fit in a register
// if no optsize is specified.
if (all_of(ResultTypes, [&](const auto &KV) {
return SwitchLookupTable::WouldFitInRegister(
DL, CR.getUpper().getLimitedValue(), KV.second /* ResultType */);
})) {
// The default branch is unreachable after we enlarge the lookup table.
// Adjust DefaultIsReachable to reuse code path.
TableSize = CR.getUpper().getZExtValue();
DefaultIsReachable = false;
}
}

const bool GeneratingCoveredLookupTable = (MaxTableSize == TableSize);
if (!DefaultIsReachable || GeneratingCoveredLookupTable) {
Builder.CreateBr(LookupBB);
if (DTU)
Expand Down
94 changes: 80 additions & 14 deletions llvm/test/Transforms/SimplifyCFG/switch_mask.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,18 @@

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

declare i1 @foo()

; https://alive2.llvm.org/ce/z/tuxLhJ
define i1 @switch_lookup_with_small_i1(i64 %x) {
; CHECK-LABEL: @switch_lookup_with_small_i1(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[AND:%.*]] = and i64 [[X:%.*]], 15
; CHECK-NEXT: [[TMP0:%.*]] = icmp ult i64 [[AND]], 11
; CHECK-NEXT: [[SWITCH_CAST:%.*]] = trunc i64 [[AND]] to i11
; CHECK-NEXT: [[SWITCH_SHIFTAMT:%.*]] = mul nuw nsw i11 [[SWITCH_CAST]], 1
; CHECK-NEXT: [[SWITCH_DOWNSHIFT:%.*]] = lshr i11 -1018, [[SWITCH_SHIFTAMT]]
; CHECK-NEXT: [[SWITCH_MASKED:%.*]] = trunc i11 [[SWITCH_DOWNSHIFT]] to i1
; CHECK-NEXT: [[TMP1:%.*]] = select i1 [[TMP0]], i1 [[SWITCH_MASKED]], i1 false
; CHECK-NEXT: ret i1 [[TMP1]]
; CHECK-NEXT: [[SWITCH_CAST:%.*]] = trunc i64 [[AND]] to i16
; CHECK-NEXT: [[SWITCH_SHIFTAMT:%.*]] = mul nuw nsw i16 [[SWITCH_CAST]], 1
; CHECK-NEXT: [[SWITCH_DOWNSHIFT:%.*]] = lshr i16 1030, [[SWITCH_SHIFTAMT]]
; CHECK-NEXT: [[SWITCH_MASKED:%.*]] = trunc i16 [[SWITCH_DOWNSHIFT]] to i1
; CHECK-NEXT: ret i1 [[SWITCH_MASKED]]
;
entry:
%and = and i64 %x, 15
Expand All @@ -37,13 +37,11 @@ define i8 @switch_lookup_with_small_i8(i64 %x) {
; CHECK-LABEL: @switch_lookup_with_small_i8(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[REM:%.*]] = urem i64 [[X:%.*]], 5
; CHECK-NEXT: [[TMP0:%.*]] = icmp ult i64 [[REM]], 3
; CHECK-NEXT: [[SWITCH_CAST:%.*]] = trunc i64 [[REM]] to i24
; CHECK-NEXT: [[SWITCH_SHIFTAMT:%.*]] = mul nuw nsw i24 [[SWITCH_CAST]], 8
; CHECK-NEXT: [[SWITCH_DOWNSHIFT:%.*]] = lshr i24 460303, [[SWITCH_SHIFTAMT]]
; CHECK-NEXT: [[SWITCH_MASKED:%.*]] = trunc i24 [[SWITCH_DOWNSHIFT]] to i8
; CHECK-NEXT: [[TMP1:%.*]] = select i1 [[TMP0]], i8 [[SWITCH_MASKED]], i8 0
; CHECK-NEXT: ret i8 [[TMP1]]
; CHECK-NEXT: [[SWITCH_CAST:%.*]] = trunc i64 [[REM]] to i40
; CHECK-NEXT: [[SWITCH_SHIFTAMT:%.*]] = mul nuw nsw i40 [[SWITCH_CAST]], 8
; CHECK-NEXT: [[SWITCH_DOWNSHIFT:%.*]] = lshr i40 460303, [[SWITCH_SHIFTAMT]]
; CHECK-NEXT: [[SWITCH_MASKED:%.*]] = trunc i40 [[SWITCH_DOWNSHIFT]] to i8
; CHECK-NEXT: ret i8 [[SWITCH_MASKED]]
;
entry:
%rem = urem i64 %x, 5
Expand Down Expand Up @@ -107,3 +105,71 @@ lor.end:
%0 = phi i8 [ 15, %sw.bb0 ], [ 6, %sw.bb1 ], [ 7, %sw.bb2 ], [ 0, %default ]
ret i8 %0
}

; Negative test: The default branch is unreachable, also it has no result.
define i1 @switch_lookup_with_small_i1_default_unreachable(i32 %x) {
; CHECK-LABEL: @switch_lookup_with_small_i1_default_unreachable(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[AND:%.*]] = and i32 [[X:%.*]], 15
; CHECK-NEXT: ret i1 false
;
entry:
%and = and i32 %x, 15
switch i32 %and, label %default [
i32 4, label %phi.end
i32 2, label %phi.end
i32 10, label %phi.end
i32 9, label %phi.end
i32 1, label %sw.bb1.i
i32 3, label %sw.bb1.i
i32 5, label %sw.bb1.i
i32 0, label %sw.bb1.i
i32 6, label %sw.bb1.i
i32 7, label %sw.bb1.i
i32 8, label %sw.bb1.i
]

sw.bb1.i: ; preds = %entry, %entry, %entry, %entry, %entry, %entry, %entry
br label %phi.end

default: ; preds = %entry
unreachable
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand. The comment says the default is reachable, but here it's marked unreachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I record it reachable because DefaultIsReachable is true for this case, and there is some case, such as i32 10 will hit the default branch. Does this not fit the usual description?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the default block starts with an unreachable instruction, DefaultIsReachable will be false:

llvm-project/llvm/lib/Transforms/Utils/SimplifyCFG.cpp

Lines 6601 to 6602 in 6477b41

const bool DefaultIsReachable =
!isa<UnreachableInst>(SI->getDefaultDest()->getFirstNonPHIOrDbg());

I was thinking of something like this, where the default doesn't yield a constant result, so we can't grow the table:

bool g(int x);

bool f(int x) {
    switch (x % 8) {
    case 0: return true;
    case 1: return false;
    case 2: return false;
    case 3: return false;
    case 4: return true;
    case 5: return false;
    case 6: return true;

    default: return g(x);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks very much for detail case, I'll update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add switch_lookup_with_small_i1_default_nonconst for above case, thanks @zmodem


phi.end: ; preds = %sw.bb1.i, %entry, %entry, %entry, %entry
%retval = phi i1 [ false, %sw.bb1.i ], [ false, %entry ], [ false, %entry ], [ false, %entry ], [ false, %entry ]
ret i1 %retval
}

; Negative test: The result in default reachable, but its value is not const.
define i1 @switch_lookup_with_small_i1_default_nonconst(i64 %x) {
; CHECK-LABEL: @switch_lookup_with_small_i1_default_nonconst(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[AND:%.*]] = and i64 [[X:%.*]], 15
; CHECK-NEXT: switch i64 [[AND]], label [[DEFAULT:%.*]] [
; CHECK-NEXT: i64 10, label [[LOR_END:%.*]]
; CHECK-NEXT: i64 1, label [[LOR_END]]
; CHECK-NEXT: i64 2, label [[LOR_END]]
; CHECK-NEXT: ]
; CHECK: default:
; CHECK-NEXT: [[CALL:%.*]] = tail call i1 @foo()
; CHECK-NEXT: br label [[LOR_END]]
; CHECK: lor.end:
; CHECK-NEXT: [[TMP0:%.*]] = phi i1 [ true, [[ENTRY:%.*]] ], [ [[CALL]], [[DEFAULT]] ], [ true, [[ENTRY]] ], [ true, [[ENTRY]] ]
; CHECK-NEXT: ret i1 [[TMP0]]
;
entry:
%and = and i64 %x, 15
switch i64 %and, label %default [
i64 10, label %lor.end
i64 1, label %lor.end
i64 2, label %lor.end
]

default: ; preds = %entry
%call = tail call i1 @foo()
br label %lor.end

lor.end: ; preds = %entry, %entry, %entry, %default
%0 = phi i1 [ true, %entry ], [ %call, %default ], [ true, %entry ], [ true, %entry ]
ret i1 %0
}