Skip to content

Commit

Permalink
[SimplifyCFG] Delete the unnecessary range check for small mask opera…
Browse files Browse the repository at this point in the history
…tion

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 #65120
Reviewed By: zmodem, nikic, RKSimon
  • Loading branch information
vfdff committed Oct 26, 2023
1 parent 925f462 commit 5e07481
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 16 deletions.
22 changes: 20 additions & 2 deletions llvm/lib/Transforms/Utils/SimplifyCFG.cpp
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,25 @@ static bool SwitchToLookupTable(SwitchInst *SI, IRBuilder<> &Builder,

BranchInst *RangeCheckBranch = nullptr;

// Grow the table to cover all possible index values to avoid the range check.
if (UseSwitchConditionAsTableIndex) {
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 when 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
24 changes: 10 additions & 14 deletions llvm/test/Transforms/SimplifyCFG/switch_mask.ll
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,11 @@ 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 +35,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

6 comments on commit 5e07481

@DavidSpickett
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change has broken 2 stage AArch64 builds:

/home/tcwg-buildbot/worker/clang-aarch64-sve-vls-2stage/stage1.install/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/tcwg-buildbot/worker/clang-aarch64-sve-vls-2stage/stage2/lib/Transforms/IPO -I/home/tcwg-buildbot/worker/clang-aarch64-sve-vls-2stage/llvm/llvm/lib/Transforms/IPO -I/home/tcwg-buildbot/worker/clang-aarch64-sve-vls-2stage/stage2/include -I/home/tcwg-buildbot/worker/clang-aarch64-sve-vls-2stage/llvm/llvm/include -mcpu=neoverse-512tvb -msve-vector-bits=256 -mllvm -treat-scalable-fixed-error-as-warning=false -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -std=c++17 -MD -MT lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/Attributor.cpp.o -MF lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/Attributor.cpp.o.d -o lib/Transforms/IPO/CMakeFiles/LLVMipo.dir/Attributor.cpp.o -c /home/tcwg-buildbot/worker/clang-aarch64-sve-vls-2stage/llvm/llvm/lib/Transforms/IPO/Attributor.cpp
clang++: ../llvm/llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6121: (anonymous namespace)::SwitchLookupTable::SwitchLookupTable(Module &, uint64_t, ConstantInt *, const SmallVectorImpl<std::pair<ConstantInt *, Constant *>> &, Constant *, const DataLayout &, const StringRef &): Assertion `DefaultValue && "Need a default value to fill the lookup table holes."' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:

https://lab.llvm.org/buildbot/#/builders/176/builds/6529
https://lab.llvm.org/buildbot/#/builders/198/builds/5563
https://lab.llvm.org/buildbot/#/builders/179/builds/8212

I'll get you the reproducer from the non-sve AArch64 build as it's the simplest.

@DavidSpickett
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I see a revert already. Let me know if you still need the reproducer.

@vfdff
Copy link
Contributor Author

@vfdff vfdff commented on 5e07481 Oct 27, 2023

Choose a reason for hiding this comment

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

Yes, that is great if you can help to reproduce, thanks very much

@vfdff
Copy link
Contributor Author

@vfdff vfdff commented on 5e07481 Oct 27, 2023

Choose a reason for hiding this comment

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

https://lab.llvm.org/buildbot/#/builders/176/builds/6529
https://lab.llvm.org/buildbot/#/builders/198/builds/5563
https://lab.llvm.org/buildbot/#/builders/179/builds/8212
I'll get you the reproducer from the non-sve AArch64 build as it's the simplest.

I can't see anything from above link, need I request some extra right ? @DavidSpickett

@DavidSpickett
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's the AArch64 reproducer: Attributor-bcec6c.zip

Same steps as the https://lab.llvm.org/buildbot/#/builders/179/builds/8212 build, but checked out at 5e07481d4240b5e8fd85f9b92df30849606c2af0.

@vfdff
Copy link
Contributor Author

@vfdff vfdff commented on 5e07481 Oct 27, 2023

Choose a reason for hiding this comment

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

I can reproduce now, thanks very much

Please sign in to comment.