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

(a > b) | (a < b) is not simplified only for the case b=0 #62586

Closed
k-arrows opened this issue May 7, 2023 · 4 comments
Closed

(a > b) | (a < b) is not simplified only for the case b=0 #62586

k-arrows opened this issue May 7, 2023 · 4 comments

Comments

@k-arrows
Copy link

k-arrows commented May 7, 2023

https://godbolt.org/z/cbPj6zfjx

int foo(int a)
{
    return (a > 0) | (a < 0);
}

int bar(int a)
{
    return (a != 0);
}
foo(int):                                # @foo(int)
        xor     eax, eax
        test    edi, edi
        setg    al
        shr     edi, 31
        or      eax, edi
        ret
bar(int):                                # @bar(int)
        xor     eax, eax
        test    edi, edi
        setne   al
        ret
@RKSimon
Copy link
Collaborator

RKSimon commented May 7, 2023

define i32 @foo(i32 %0) {
  %2 = icmp sgt i32 %0, 0
  %3 = zext i1 %2 to i32
  %4 = lshr i32 %0, 31
  %5 = or i32 %4, %3
  ret i32 %5
}

@k-arrows
Copy link
Author

k-arrows commented May 7, 2023

https://alive2.llvm.org/ce/z/33HzjE

define i32 @src(i32 %0) {
%1:
  %2 = icmp sgt i32 %0, 0
  %3 = zext i1 %2 to i32
  %4 = lshr i32 %0, 31
  %5 = or i32 %4, %3
  ret i32 %5
}
=>
define i32 @tgt(i32 %0) {
%1:
  %2 = icmp ne i32 %0, 0
  %3 = zext i1 %2 to i32
  ret i32 %3
}
Transformation seems to be correct!

@XChy
Copy link
Member

XChy commented Jul 3, 2023

Patch for the simplification:patch

goldsteinn pushed a commit that referenced this issue Jul 6, 2023
…NFC)

Tests for an upcoming  (A > 0) | (A < 0) -> zext (A != 0) fold.
Related issue:
[[ #62586 | (a > b) | (a < b) is not simplified only for the case b=0 ]]

Differential Revision: https://reviews.llvm.org/D154089
goldsteinn pushed a commit that referenced this issue Jul 6, 2023
[InstCombine] Transform (A > 0) | (A < 0) -> zext (A != 0) fold

This extends **foldCastedBitwiseLogic** to handle the similar cases.

Actually, for `(A > B) | (A < B)`, when B != 0, it can be optimized to `zext( A != B )` by **foldAndOrOfICmpsUsingRanges**.
However, when B = 0, **transformZExtICmp** will transform `zext(A < 0) to i32` into `A << 31`,
which cannot be optimized by **foldAndOrOfICmpsUsingRanges**.

Because I'm new to LLVM and has no concise knowledge about how LLVM decides the order of optimization,
I choose to extend **foldCastedBitwiseLogic** to fold `( A << (X - 1) ) | ((A > 0) zext to iX) -> (A != 0) zext to iX`.

And the equivalent fold follows:
```
 A << (X - 1) ) | ((A > 0) zext to iX
  -> A < 0 | A > 0
  -> (A != 0) zext to iX
```

It's proved by [[https://alive2.llvm.org/ce/z/33HzjE|alive-tv]]

Related issue:
[[#62586  | (a > b) | (a < b) is not simplified only for the case b=0 ]]

Reviewed By: goldstein.w.n

Differential Revision: https://reviews.llvm.org/D154126
@k-arrows
Copy link
Author

k-arrows commented Jul 8, 2023

I will close this issue since the fix has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants