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.

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

Missed optimization for bitset based equality #65120

Closed
bmaurer opened this issue Aug 30, 2023 · 1 comment · Fixed by #65835
Closed

Missed optimization for bitset based equality #65120

bmaurer opened this issue Aug 30, 2023 · 1 comment · Fixed by #65835

Comments

@bmaurer
Copy link

bmaurer commented Aug 30, 2023

https://godbolt.org/z/nTseoW9z1

bool test(unsigned long long x) {
    auto mask = x & 0xf;
    return (mask == 10 || mask == 1 || mask == 2);
}

clang generates:

test(unsigned long long):                               # @test(unsigned long long)
        and     edi, 15
        cmp     rdi, 11
        setb    cl
        mov     eax, 1030
        bt      eax, edi
        setb    al
        and     al, cl
        ret

the comparison against 11 is unnecessary given the range of the mask operation. GCC emits shorter code:

test(unsigned long long):
        and     edi, 15
        mov     eax, 1030
        bt      rax, rdi
        setc    al
        ret
@RKSimon
Copy link
Collaborator

RKSimon commented Aug 31, 2023

simplifycfg folds:

define i1 @test(i64 noundef %x) {
entry:
  %and = and i64 %x, 15
  switch i64 %and, label %lor.rhs [
    i64 10, label %lor.end
    i64 1, label %lor.end
    i64 2, label %lor.end
  ]

lor.rhs:                                          ; preds = %entry
  br label %lor.end

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

to:

define i1 @test(i64 noundef %x) {
entry:
  %and = and i64 %x, 15
  %0 = icmp ult i64 %and, 11
  %switch.cast = trunc i64 %and to i11
  %switch.shiftamt = mul nuw nsw i11 %switch.cast, 1
  %switch.downshift = lshr i11 -1018, %switch.shiftamt
  %switch.masked = trunc i11 %switch.downshift to i1
  %1 = select i1 %0, i1 %switch.masked, i1 false
  ret i1 %1
}

which instcombine then folds to:

define i1 @test(i64 noundef %x){
entry:
  %and = and i64 %x, 15
  %0 = icmp ult i64 %and, 11
  %switch.cast = trunc i64 %and to i11
  %switch.downshift = lshr i11 -1018, %switch.cast
  %1 = and i11 %switch.downshift, 1
  %switch.masked = icmp ne i11 %1, 0
  %2 = select i1 %0, i1 %switch.masked, i1 false
  ret i1 %2
}

@vfdff vfdff closed this as completed in 5e07481 Oct 26, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this issue Oct 26, 2023
…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 reopened this Oct 27, 2023
vfdff added a commit to vfdff/llvm-project that referenced this issue Nov 2, 2023
…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
@vfdff vfdff closed this as completed in 7c4180a Nov 3, 2023
zmodem added a commit that referenced this issue Nov 7, 2023
…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.
vfdff added a commit to vfdff/llvm-project that referenced this issue Nov 8, 2023
…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

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 llvm#71329.
qihangkong pushed a commit to rvgpu/llvm that referenced this issue Apr 18, 2024
…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 issue Apr 18, 2024
…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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants