Bug 58564 - [4.7 Regression] possible wrong code bug at -O0
Summary: [4.7 Regression] possible wrong code bug at -O0
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.9.0
: P3 normal
Target Milestone: 4.7.4
Assignee: Jakub Jelinek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-28 18:10 UTC by John Regehr
Modified: 2014-05-07 16:20 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2013-09-30 00:00:00


Attachments
gcc49-pr58564.patch (903 bytes, patch)
2013-09-30 09:41 UTC, Jakub Jelinek
Details | Diff
gcc49-pr58564-nonnegative.patch (435 bytes, patch)
2013-09-30 09:56 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Regehr 2013-09-28 18:10:52 UTC
regehr@john-home ~ $ clang -O0 -w small.c ; ./a.out
0
regehr@john-home ~ $ gcc-4.4 -O0 -w small.c ; ./a.out
0
regehr@john-home ~ $ gcc-4.6 -O0 -w small.c ; ./a.out
0
regehr@john-home ~ $ gcc-4.7 -O0 -w small.c ; ./a.out
1
regehr@john-home ~ $ gcc -O0 -w small.c ; ./a.out
1
regehr@john-home ~ $ cat small.c 
int printf(const char *, ...);
int a, b;
short *c;
short **d = &c;
int main() {
  b = (0, 0 > ((&c == d) & (1 && a ^ 1))) | 0U;
  printf("%d\n", b);
  return 0;
}
regehr@john-home ~ $ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/home/regehr/z/compiler-install/gcc-r203005-install/libexec/gcc/x86_64-unknown-linux-gnu/4.9.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: /home/regehr/z/compiler-source/gcc/configure --prefix=/home/regehr/z/compiler-install/gcc-r203005-install --enable-languages=c,c++ --disable-multilib
Thread model: posix
gcc version 4.9.0 20130928 (experimental) (GCC)
Comment 1 Mikael Pettersson 2013-09-28 21:05:42 UTC
4.6 works, 4.7 to trunk don't, started with Kai's r176563.
Comment 2 Kai Tietz 2013-09-28 21:25:30 UTC
This sample is invalid.  And before 4.7 there seems to be wrong result.

You missed the following here in your expression
0 > ((&c == d) & (1 && a ^ 1))

The term (1 && a ^ 1)  is not the same as (1 & (a ^ 1))

instead it means in fact (1 != 0 && (a ^ 1) != 0.  By this term reduces correct to:
 a ^ 1 != 0 and this is indentical to a != 1.

by this we see 0 > (&c == d) & (a != 1).  And this is exactly that what AST generates (see here -fdump-tree-original).

So error is invalid.  But indeed this testcase demonstrate, that we seem to have still pointer-arthimetic optimization issues, due the term (&c == d)  was resolved ...
Comment 3 John Regehr 2013-09-29 03:03:39 UTC
Kai, this is a real bug, please reopen it.

Here is what I get out of -fdump-tree-original:

  b = (int) (d == &c && a != 1);

This is wrong.

One way to illustrate the problem is to remove the useless comma operator from the test case, which now looks like this (I've also added parens around the ^):

int printf(const char *, ...);
int a, b;
short *c;
short **d = &c;
int main() {
  b = (0 > ((&c == d) & (1 && (a ^ 1)))) | 0U;
  printf("%d\n", b);
  return 0;
}

Now -fdump-tree-original does a better job:

 b = (int) (d == &c && a != 1) < 0;

And now all compilers agree on the output:

[regehr@imp r102]$ gcc-4.4 small2.c ; ./a.out 
0
[regehr@imp r102]$ gcc-4.5 small2.c ; ./a.out 
0
[regehr@imp r102]$ gcc-4.6 small2.c ; ./a.out 
0
[regehr@imp r102]$ gcc small2.c ; ./a.out 
0
[regehr@imp r102]$ clang small2.c ; ./a.out 
0
Comment 4 Mikael Pettersson 2013-09-29 09:23:38 UTC
(In reply to John Regehr from comment #3)
> Kai, this is a real bug, please reopen it.
> 
> Here is what I get out of -fdump-tree-original:
> 
>   b = (int) (d == &c && a != 1);
> 
> This is wrong.
> 
> One way to illustrate the problem is to remove the useless comma operator
> from the test case, which now looks like this (I've also added parens around
> the ^):

I can confirm this observation.  Adding parentheses around the "a ^ 1" makes no difference, but removing the comma operator and its left operand "0, " makes 4.7 to trunk compute the same result as 4.6 and older did.
Comment 5 Kai Tietz 2013-09-30 06:53:10 UTC
Sorry, missed that outer condition is 0 > ... boolean-typed-expr.

Can confirm this issue.  Btw this condition has to be always false for unsigned 1-bit precision typed integrals.
Comment 6 Jakub Jelinek 2013-09-30 08:43:46 UTC
I think the bug is in the
      /* A < 0 ? <sign bit of A> : 0 is simply (A & <sign bit of A>).  */
      if (TREE_CODE (arg0) == LT_EXPR
          && integer_zerop (TREE_OPERAND (arg0, 1))
          && integer_zerop (op2)
          && (tem = sign_bit_p (TREE_OPERAND (arg0, 0), arg1)))
...
transformation in fold_ternary_loc or sign_bit_p or combination thereof.
We get here with arg0 ((int) (d == &c && a != 1)) < 0, arg1 1U and arg2 0U,
sign_bit_p (surprisingly) looks through zero extensions (of bool to int)
and returns (d == &c && a != 1) and we incorrectly fold it as
(d == &c && a != 1) & 1.
Comment 7 Jakub Jelinek 2013-09-30 09:41:51 UTC
Created attachment 30932 [details]
gcc49-pr58564.patch

Untested fix.

Note, this patch doesn't attempt to fold the zero extension of something < 0
into 0 resp. side-effects of something, 0, because I think we are generally
trying to move away from adding further new stuff to fold-const.c, and rather
should improve GIMPLE optimizations.
Comment 8 Jakub Jelinek 2013-09-30 09:56:00 UTC
Created attachment 30933 [details]
gcc49-pr58564-nonnegative.patch

Actually, teaching fold that it should fold that < 0 into 0 is easy, just needs making some checks less strict (they were considering only INTEGER_TYPE, forgetting about BOOLEAN_TYPE or ENUMERAL_TYPE which IMHO can be handled the same).  This latter patch I'm obviously not going to propose for the older branches.
Comment 9 Jakub Jelinek 2013-09-30 20:15:22 UTC
Author: jakub
Date: Mon Sep 30 20:15:20 2013
New Revision: 203042

URL: http://gcc.gnu.org/viewcvs?rev=203042&root=gcc&view=rev
Log:
	PR middle-end/58564
	* fold-const.c (fold_ternary_loc): For A < 0 : <sign bit of A> : 0
	optimization, punt if sign_bit_p looked through any zero extension.

	* gcc.c-torture/execute/pr58564.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr58564.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c
    trunk/gcc/testsuite/ChangeLog
Comment 10 Jakub Jelinek 2013-09-30 20:16:15 UTC
Author: jakub
Date: Mon Sep 30 20:16:14 2013
New Revision: 203043

URL: http://gcc.gnu.org/viewcvs?rev=203043&root=gcc&view=rev
Log:
	PR middle-end/58564
	* fold-const.c (fold_ternary_loc): For A < 0 : <sign bit of A> : 0
	optimization, punt if sign_bit_p looked through any zero extension.

	* gcc.c-torture/execute/pr58564.c: New test.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.c-torture/execute/pr58564.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/fold-const.c
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
Comment 11 Jakub Jelinek 2013-09-30 20:17:09 UTC
Author: jakub
Date: Mon Sep 30 20:17:07 2013
New Revision: 203044

URL: http://gcc.gnu.org/viewcvs?rev=203044&root=gcc&view=rev
Log:
	PR middle-end/58564
	* fold-const.c (tree_unary_nonnegative_warnv_p): Use
	INTEGRAL_TYPE_P (t) instead of TREE_CODE (t) == INTEGER_TYPE.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c
Comment 12 Jakub Jelinek 2013-09-30 20:17:56 UTC
Should be fixed for 4.8.2+ so far.
Comment 13 Jakub Jelinek 2014-05-07 16:06:10 UTC
Author: jakub
Date: Wed May  7 16:05:38 2014
New Revision: 210174

URL: http://gcc.gnu.org/viewcvs?rev=210174&root=gcc&view=rev
Log:
	Backported from mainline
	2013-09-30  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/58564
	* fold-const.c (fold_ternary_loc): For A < 0 : <sign bit of A> : 0
	optimization, punt if sign_bit_p looked through any zero extension.

	* gcc.c-torture/execute/pr58564.c: New test.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.c-torture/execute/pr58564.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/fold-const.c
    branches/gcc-4_7-branch/gcc/testsuite/ChangeLog
Comment 14 Jakub Jelinek 2014-05-07 16:20:56 UTC
Fixed.