[llvm] [SimplifyCFG] Fix the compile crash for invalid upper bound value (PR #71351)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 8 17:41:18 PST 2023
https://github.com/vfdff updated https://github.com/llvm/llvm-project/pull/71351
>From ad0f99adccaae56f7ce26606e9599535d8723e52 Mon Sep 17 00:00:00 2001
From: zhongyunde 00443407 <zhongyunde at huawei.com>
Date: Fri, 1 Sep 2023 23:05:45 -0400
Subject: [PATCH 1/2] [SimplifyCFG] Delete the unnecessary range check for
small mask operation
When the small mask value little than 64, we can eliminate the checking
for upper limit of the range by enlarge the lookup table size to the maximum
index value. (Then the final table size grows to the next pow2 value)
```
bool f(unsigned x) {
switch (x % 8) {
case 0: return 1;
case 1: return 0;
case 2: return 0;
case 3: return 1;
case 4: return 1;
case 5: return 0;
case 6: return 1;
// This would remove the range check: case 7: return 0;
}
return 0;
}
```
Use WouldFitInRegister instead of fitsInLegalInteger to support
more result type beside bool.
Fixes https://github.com/llvm/llvm-project/issues/65120
Note:
For '%add = add nuw i32 %x, 1', we can infer the LowerBound is 1,
but the UpperBound is wrapped to 0 in computeConstantRange.
so we can't assume the UpperBound is valid bound when its value is 0.
Fix https://github.com/llvm/llvm-project/issues/71329.
---
llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 25 +++-
.../Transforms/SimplifyCFG/switch_mask.ll | 126 ++++++++++++++++--
2 files changed, 135 insertions(+), 16 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 5ef3a5292af545c..4dae52a8ecffdf6 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -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();
@@ -6631,6 +6630,28 @@ 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) {
+ 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.
+ const uint64_t UpperBound = CR.getUpper().getLimitedValue();
+ if (!CR.isUpperWrapped() && all_of(ResultTypes, [&](const auto &KV) {
+ return SwitchLookupTable::WouldFitInRegister(
+ DL, UpperBound, KV.second /* ResultType */);
+ })) {
+ // The default branch is unreachable after we enlarge the lookup table.
+ // Adjust DefaultIsReachable to reuse code path.
+ TableSize = UpperBound;
+ DefaultIsReachable = false;
+ }
+ }
+
+ const bool GeneratingCoveredLookupTable = (MaxTableSize == TableSize);
if (!DefaultIsReachable || GeneratingCoveredLookupTable) {
Builder.CreateBr(LookupBB);
if (DTU)
diff --git a/llvm/test/Transforms/SimplifyCFG/switch_mask.ll b/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
index 8c97a0660d07074..d34d91532d41eaa 100644
--- a/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
+++ b/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
@@ -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
@@ -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
@@ -107,3 +105,103 @@ 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
+
+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
+}
+
+; Negative test: The upper bound index of switch is swapped.
+define void @switch_lookup_with_nonconst_range(i32 %x, i1 %cond) {
+; CHECK-LABEL: @switch_lookup_with_nonconst_range(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[FOR_PREHEADER:%.*]]
+; CHECK: for.preheader:
+; CHECK-NEXT: br i1 [[COND:%.*]], label [[FOR_PREHEADER]], label [[LOR_END:%.*]]
+; CHECK: lor.end:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %for.preheader
+
+for.preheader: ; preds = %for.preheader, %entry
+ %add = add nuw i32 %x, 1 ; the UpperBound is unconfirmed
+ br i1 %cond, label %for.preheader, label %for.end
+
+for.end: ; preds = %for.preheader
+ switch i32 %add, label %default [
+ i32 0, label %lor.end
+ i32 1, label %lor.end
+ i32 5, label %lor.end
+ ]
+
+default: ; preds = %for.end
+ br label %lor.end
+
+lor.end: ; preds = %default, %for.end, %for.end, %for.end
+ %retval.0.i.i = phi i32 [ 0, %default ], [ 0, %for.end ], [ 0, %for.end ], [ 0, %for.end ]
+ ret void
+}
>From 46afd129e45e5295facb6664fb2acae5f3de48ec Mon Sep 17 00:00:00 2001
From: zhongyunde 00443407 <zhongyunde at huawei.com>
Date: Wed, 8 Nov 2023 20:21:23 -0500
Subject: [PATCH 2/2] [SimplifyCFG] update test with different values for phi
node
---
llvm/test/Transforms/SimplifyCFG/switch_mask.ll | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/llvm/test/Transforms/SimplifyCFG/switch_mask.ll b/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
index d34d91532d41eaa..17f7b583ee840ed 100644
--- a/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
+++ b/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
@@ -180,8 +180,17 @@ define void @switch_lookup_with_nonconst_range(i32 %x, i1 %cond) {
; CHECK-NEXT: entry:
; CHECK-NEXT: br label [[FOR_PREHEADER:%.*]]
; CHECK: for.preheader:
-; CHECK-NEXT: br i1 [[COND:%.*]], label [[FOR_PREHEADER]], label [[LOR_END:%.*]]
+; CHECK-NEXT: [[ADD:%.*]] = add nuw i32 [[X:%.*]], 1
+; CHECK-NEXT: br i1 [[COND:%.*]], label [[FOR_PREHEADER]], label [[FOR_END:%.*]]
+; CHECK: for.end:
+; CHECK-NEXT: [[TMP0:%.*]] = icmp ult i32 [[ADD]], 6
+; CHECK-NEXT: br i1 [[TMP0]], label [[SWITCH_LOOKUP:%.*]], label [[LOR_END:%.*]]
+; CHECK: switch.lookup:
+; CHECK-NEXT: [[SWITCH_GEP:%.*]] = getelementptr inbounds [6 x i32], ptr @switch.table.switch_lookup_with_nonconst_range, i32 0, i32 [[ADD]]
+; CHECK-NEXT: [[SWITCH_LOAD:%.*]] = load i32, ptr [[SWITCH_GEP]], align 4
+; CHECK-NEXT: br label [[LOR_END]]
; CHECK: lor.end:
+; CHECK-NEXT: [[RETVAL_0_I_I:%.*]] = phi i32 [ [[SWITCH_LOAD]], [[SWITCH_LOOKUP]] ], [ 1, [[FOR_END]] ]
; CHECK-NEXT: ret void
;
entry:
@@ -202,6 +211,6 @@ default: ; preds = %for.end
br label %lor.end
lor.end: ; preds = %default, %for.end, %for.end, %for.end
- %retval.0.i.i = phi i32 [ 0, %default ], [ 0, %for.end ], [ 0, %for.end ], [ 0, %for.end ]
+ %retval.0.i.i = phi i32 [ 1, %default ], [ 0, %for.end ], [ 0, %for.end ], [ 0, %for.end ]
ret void
}
More information about the llvm-commits
mailing list