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
possible wrong code bug #17847
Comments
John, thanks for cc'ing me on this report. I suspect that the code is invalid because as far as I know the standard doesn't like code like "a ?: b", where the middle term is missing. |
The missing argument to the ternary operator is a GCC idiom that I believe Clang attempts to implement faithfully. x?:z simply means x?x:z |
I see; thanks John. I was able to reduce it a bit more and got rid of fn2 (see below). int printf (const char *, ...); int void int |
Oh, fn1 should be removed too: int printf (const char *, ...); void int |
Thanks! I was lazy and figured that if C-Reduce couldn't get rid of those functions, I couldn't either. |
Here's a different, smaller test case that looks similar enough that I'll put it here instead of making a new PR. [regehr@imp r105]$ cat small.c int main() { |
It should be different as only the trunk miscompiles it at -O2, while all versions of clang miscompile the initial testcase. |
Here is another testcase that should point to the same issue as the above testcase. Since both miscompilations go away with -fno-vectorize, this should be a vectorizer bug. int printf (const char *, ...); int a, b, c, d; int |
The first three snippets are not related to the vectorizers: Compiling them with:
shows no vectorized code. And compiling them with the vectorizers off shows the same failure behavior:
The last two examples are indeed a bug in the loop vectorizer. I will commit a fix for them soon. Basically, we were taking a reduction like this: for.body3.1: ; preds = %for.body3.1, %for.inc7 for.inc7.1: ; preds = %for.body3.1 And where generating this. The problem is that the external user was not using the “last” value in the reduction. So we would loose 3 "(add 1)”s. vector.body23: ; preds = %vector.body23, %for.inc7 middle.block24: ; preds = %vector.body23 I have filed PR 17498 to track the vectorizer bug. |
Hi. What's the status of this bug? |
The test in comment 4 still reproduces: int printf(const char *, ...); It prints different values at -O2 and -O0. |
Looks like a bug in gvn /build/bin/clang test.c -o test.ll -m64 -emit-llvm -S -O0 prints ffffffff |
input to gvn misoptimizes this. |
Actually, I think it's an LSR bug. Here's what I get: [morbo:llvm] opt -S -basicaa -gvn test2.ll -o - | llc -o test2.s -relocation-model pic -disable-fp-elim && clang test2.s -o test2 && ./test2 |
rdar://problem/15464466 #17847 : LSR miscompile |
As far as I understand, the test exploits undefined behaviour (signed overflow), which can't be considered as a compiler bug. |
Michael, UBSAN doesn't think there's a signed overflow, can you explain why you think there is one? |
Hi John, You could add a printf into the loop and it would become apparent: int printf(const char *, ...); The output would be: Also one could figure that out from the loop condition: (h = 0; h >= 0; h++). |
Hi Michael, most people believe that ++h cannot overflow when h is a char because it is promoted to int, incremented, and then converted back to char. I believe this issue is not 100% clear in the standard, but my the view above is shared by the Clang developers, which explains why UBSAN does not complain here. |
Yes, I agree with John. This also applies to signed short. |
Good to know, thanks! |
I looked into LSR to figure out, why is this happening and here is what I found. When LSR replaces IV with a new one, it uses i32 instead of original i8. As far as I understand, it is the widest type used in computations involving the IV, and the reason why we can do this is that we later can simply ignore higher bits if we need a narrower type. That usually works fine, but in our case the ultimate use is also i32, so we don't add truncate before it, and that causes the fail. I.e. the correct conversion sequence here should be i32->i8->i32, but LSR currently loses the middle part and thus doesn't emit needed trunc/sext, which seems to be the rootcause. |
Fixed in r203719. |
mentioned in issue llvm/llvm-bugzilla-archive#17498 |
mentioned in issue llvm/llvm-bugzilla-archive#18165 |
Extended Description
Well this one is weird since fn2 isn't even called, but it can't be removed or the bad behavior goes away.
[regehr@imp r104]$ clang -w -O0 small.c ; ./a.out
ffffffff
[regehr@imp r104]$ clang -w -O small.c ; ./a.out
ff
[regehr@imp r104]$ gcc -w -O small.c ; ./a.out
ffffffff
[regehr@imp r104]$ cat small.c
int printf(const char *, ...);
int a, c, d, e, f, g, i, j;
short b;
char h;
int fn1(void)
{
return b && a > b || b < 0 && a ?: b;
}
void fn2(void) {
for (;; f = fn1())
;
}
void fn3(int p1) {
j &&(c = 0 ?: 0);
g = p1;
}
int main(void) {
for (h = 0; h >= 0; ++h) {
fn3(h);
i = d >= 0 ?: 0;
}
e = g;
e += h;
printf("%x\n", e);
return 0;
}
[regehr@imp r104]$ clang -v
clang version 3.4 (trunk 191880)
Target: x86_64-unknown-linux-gnu
Thread model: posix
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.4
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.4.6
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.4.7
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.5
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.5.3
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.6
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.6.3
Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.6
The text was updated successfully, but these errors were encountered: