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

wrong code at -Os and above on x86_64-linux-gnu #50288

Closed
zhendongsu opened this issue Jun 30, 2021 · 10 comments
Closed

wrong code at -Os and above on x86_64-linux-gnu #50288

zhendongsu opened this issue Jun 30, 2021 · 10 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla

Comments

@zhendongsu
Copy link

Bugzilla Link 50944
Resolution FIXED
Resolved on Jul 02, 2021 01:10
Version trunk
OS All
CC @aqjune,@RKSimon,@nikic,@rotateright
Fixed by commit(s) 9eb613b

Extended Description

This seems to be a recent regression.

[512] % clangtk -v
clang version 13.0.0 (https://github.com/llvm/llvm-project.git 0c96a92)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /local/suz-local/opfuzz/bin
Found candidate GCC installation: /usr/lib/gcc/i686-linux-gnu/8
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/6
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/6.5.0
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/7
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/7.5.0
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/8
Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/7.5.0
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Candidate multilib: x32;@MX32
Selected multilib: .;@m64
[513] %
[513] % clangtk -O1 small.c; ./a.out
[514] %
[514] % clangtk -Os small.c
[515] % ./a.out
Aborted
[516] %
[516] % cat small.c
static int a, b, *c = &b, d;
int main() {
int e;
for (; a < 1; a++)
if (b)
d++;
b = 0;
if (!a)
b = 0 >> e;
c = 0;
if (d != 0)
__builtin_abort ();
return 0;
}

@zhendongsu
Copy link
Author

assigned to @rotateright

@rotateright
Copy link
Contributor

Looks like something broke in instcombine with respect to poison. Taking a look now...

@rotateright
Copy link
Contributor

Something in this patch:
https://reviews.llvm.org/rG5af8bacc940243038478da1c92c3481cbdfcece3
...either exposed an existing bug or created one.

Reduced test:

define i1 @​#50944 (i1 %b) {
%s = select i1 %b, i32 poison, i32 0
%tobool = icmp ne i32 %s, 0
ret i1 %tobool
}

$ opt -instcombine selpoison.ll -S
define i1 @​f(i1 %b) {
ret i1 poison
}

...which is wrong because there's clearly no poison returned if %b is false. This was and should be simplified to 'false'.

Interestingly, the bug is not visible if I run instsimplify directly:
$ ./opt -instsimplify selpoison.ll -S
define i1 @​f(i1 %b) {
ret i1 false
}

So something is going wrong with the SimplifyQuery?

@rotateright
Copy link
Contributor

Stepped through in the debugger to see why -instcombine behaves differently: we have this FIXME:

// FIXME: Remove this workaround when freeze related patches are done.

...so we don't simplify the select directly. We end up in SimplifyICmp which then calls handleOtherCmpSelSimplifications() -

static Value *handleOtherCmpSelSimplifications(Value *TCmp, Value *FCmp,

...and that conversion to logic ops looks suspect.

@rotateright
Copy link
Contributor

@rotateright
Copy link
Contributor

*** Bug llvm/llvm-bugzilla-archive#50952 has been marked as a duplicate of this bug. ***

@rotateright
Copy link
Contributor

@aqjune
Copy link
Contributor

aqjune commented Jul 2, 2021

The final fix looks valid to me as well!

@zhendongsu
Copy link
Author

mentioned in issue llvm/llvm-bugzilla-archive#50947

@zhendongsu
Copy link
Author

mentioned in issue llvm/llvm-bugzilla-archive#50952

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants