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

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

Merged
merged 2 commits into from
Oct 26, 2023
Merged

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

merged 2 commits into from
Oct 26, 2023

Conversation

vfdff
Copy link
Contributor

@vfdff vfdff commented Sep 9, 2023 •
edited
Loading

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 its next pow2 value. Consider: https://godbolt.org/z/3qnE45YWj

Fixes #65120

@vfdff vfdff requested a review from a team as a code owner September 9, 2023 05:21
nikic
nikic reviewed Sep 15, 2023
View reviewed changes
llvm/test/Transforms/SimplifyCFG/switch_mask.ll Outdated Show resolved Hide resolved
llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
llvm/test/Transforms/SimplifyCFG/switch_mask.ll Outdated Show resolved Hide resolved
llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
nikic
nikic reviewed Sep 18, 2023
View reviewed changes
llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
@vfdff
Copy link
Contributor Author

vfdff commented Sep 28, 2023

ping ?

@vfdff vfdff requested review from nikic and RKSimon October 7, 2023 16:52
@vfdff
Copy link
Contributor Author

vfdff commented Oct 14, 2023

sorry for ping

@aeubanks aeubanks requested review from zmodem and removed request for a team October 16, 2023 20:21
@vfdff vfdff force-pushed the D159395 branch 2 times, most recently from 78e4159 to 76bde92 Compare October 19, 2023 01:36
zmodem
zmodem reviewed Oct 19, 2023
View reviewed changes
Copy link
Collaborator

@zmodem zmodem left a comment

Choose a reason for hiding this comment

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

This basically looks good to me. Just some minor comments.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp Outdated Show resolved Hide resolved
llvm/test/Transforms/SimplifyCFG/switch_mask.ll Outdated Show resolved Hide resolved
@vfdff vfdff force-pushed the D159395 branch 2 times, most recently from fa48021 to 7d66f9b Compare October 20, 2023 01:06
zmodem
zmodem approved these changes Oct 20, 2023
View reviewed changes
Copy link
Collaborator

@zmodem zmodem left a comment

Choose a reason for hiding this comment

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

lgtm
Please wait for @nikic to check too.

@vfdff vfdff force-pushed the D159395 branch 2 times, most recently from 03fb9ae to 5dcb980 Compare October 21, 2023 08:01
@vfdff vfdff requested a review from nikic October 24, 2023 02:35
nikic
nikic approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@vfdff
[SimplifyCFG] Precommit tests for PR65835
925f462
@vfdff
[SimplifyCFG] Delete the unnecessary range check for small mask opera…
5e07481 
…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 llvm#65120
Reviewed By: zmodem, nikic, RKSimon
@vfdff vfdff merged commit 5e07481 into llvm:main Oct 26, 2023
4 of 5 checks passed
@vfdff vfdff deleted the D159395 branch October 26, 2023 11:04
@nikic
Copy link
Contributor

nikic commented Oct 26, 2023

It looks like this causes assertion failures on some build bots, for example https://lab.llvm.org/buildbot/#/builders/176/builds/6529:

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.

@vfdff
Copy link
Contributor Author

vfdff commented Oct 26, 2023

ok, I revert it soon, sorry

This was referenced Oct 26, 2023
Merged
Merged
vfdff added a commit that referenced this pull request Nov 3, 2023
@vfdff
Reland [SimplifyCFG] Delete the unnecessary range check for small mas…
7c4180a 
…k operation (#70542)

Fix the compile crash when the default result has no result  for
#65835

Fixes #65120
Reviewed By: zmodem, nikic
zmodem added a commit that referenced this pull request Nov 7, 2023
@zmodem
Revert "Reland [SimplifyCFG] Delete the unnecessary range check for s…
05ed921 
…mall mask operation (#70542)"

This caused #71329

> Fix the compile crash when the default result has no result  for
> #65835
>
> Fixes #65120
> Reviewed By: zmodem, nikic

This reverts commit 7c4180a.
qihangkong pushed a commit to rvgpu/llvm that referenced this pull request Apr 18, 2024
@vfdff @zahiraam
Reland [SimplifyCFG] Delete the unnecessary range check for small mas…
4b63049 
…k operation (#70542)

Fix the compile crash when the default result has no result  for
llvm/llvm-project#65835

Fixes llvm/llvm-project#65120
Reviewed By: zmodem, nikic
qihangkong pushed a commit to rvgpu/llvm that referenced this pull request Apr 18, 2024
@zmodem @zahiraam
Revert "Reland [SimplifyCFG] Delete the unnecessary range check for s…
8afe133 
…mall mask operation (#70542)"

This caused llvm/llvm-project#71329

> Fix the compile crash when the default result has no result  for
> llvm/llvm-project#65835
>
> Fixes llvm/llvm-project#65120
> Reviewed By: zmodem, nikic

This reverts commit 7c4180a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

@zmodem zmodem zmodem approved these changes

@aeubanks aeubanks Awaiting requested review from aeubanks

@RKSimon RKSimon Awaiting requested review from RKSimon

Assignees
No one assigned
Labels
llvm:transforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missed optimization for bitset based equality
6 participants
@vfdff @nikic @DavidSpickett @zmodem @RKSimon @llvmbot