[Bug c++/50189] New: Wrong code error in -O2 compile, target independent

[Bug c++/50189] New: Wrong code error in -O2 compile, target independent

[pkoning at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

             Bug #: 50189
           Summary: Wrong code error in -O2 compile, target independent
    Classification: Unclassified
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: pkoning@gcc.gnu.org


Created attachment 25105
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25105
Test program

The attached test program compiles to wrong code with GCC 4.5.3 (and 4.5.1)
when compiled -O2.  GCC 4.6.1 gets it right.

I did some digging and determined that the problem shows up in the
-fdump-tree-all output in the .068t.vrp1 dump file; the preceding file
(.067.mergephi2) is correct.  In the generated code that corresponds to the
check_pos method, the second compare "if (m_timestamp != a_timestamp)" is
transformed into "if (3 == a_timestamp)".  If m_timestamp were of time
position_t (an enum of range 0..3) that would be correct, but m_timestamp is an
unsigned int so it can, and in our application does, have values larger than 3.

[Bug c++/50189] Wrong code error in -O2 compile, target independent

[pkoning at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #1 from Paul Koning <pkoning at gcc dot gnu.org> 2011-08-25 17:19:23 UTC ---
Created attachment 25106
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25106
gcc -v output (configure options etc.

[Bug c++/50189] Wrong code error in -O2 compile, target independent

[pkoning at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

Paul Koning <pkoning at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

--- Comment #2 from Paul Koning <pkoning at gcc dot gnu.org> 2011-08-25 17:20:41 UTC ---
I forgot to mention: the test program was run on Linux-i686, but the problem
was originally discovered and also exists on a mips64-netbsdelf targeted gcc
4.5.1.

[Bug c++/50189] [4.5 Regression] Wrong code error in -O2 compile, target independent

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
      Known to work|                            |4.6.1
   Target Milestone|---                         |4.5.5
            Summary|Wrong code error in -O2     |[4.5 Regression] Wrong code
                   |compile, target independent |error in -O2 compile,
                   |                            |target independent

[Bug middle-end/50189] [4.5 Regression] Wrong code error in -O2 compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011-08-26
          Component|c++                         |middle-end
      Known to work|                            |4.4.6
            Version|unknown                     |4.5.3
         Depends on|                            |49911
   Target Milestone|4.5.5                       |4.5.4
     Ever Confirmed|0                           |1
      Known to fail|                            |4.5.3

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-08-26 07:49:58 UTC ---
Surely related to PR49911.  That it works with 4.4, 4.6 and 4.7 is pure luck.

[Bug middle-end/50189] [4.5 Regression] Wrong code error in -O2 compile, target independent

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> 2011-08-26 08:00:54 UTC ---
Um, in 4.6+ we default to -fno-strict-enums, so I would guess that the reason
why it doesn't fail there isn't pure luck.  Of course with -fstrict-enums it
would either fail or be a pure luck.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|4.6.1                       |4.7.0
            Summary|[4.5 Regression] Wrong code |[4.5/4.6 Regression] Wrong
                   |error in -O2 compile,       |code error in -O2
                   |target independent          |[-fstrict-enums] compile,
                   |                            |target independent
      Known to fail|                            |4.6.1

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-08-26 08:26:50 UTC ---
Ah, right.  Fails on 4.6.1 with -fstrict-enums as well, but not on trunk where
it seems to work by luck.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[pkoning at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #6 from Paul Koning <pkoning at gcc dot gnu.org> 2011-09-09 19:11:01 UTC ---
I saw the note that PR/49911 is fixed and thought that might mean this one is
fixed also.  Unfortunately testing shows that is not the case, at least not in
4_5_branch.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[pkoning at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #7 from Paul Koning <pkoning at gcc dot gnu.org> 2011-10-10 20:41:35 UTC ---
Re comment 5, does "works by luck" mean that I should not look in trunk for a
fix to backport because nothing was actually fixed?

Should I just avoid all versions of GCC newer than 4.4?  The verdict "works by
luck" frankly does not give me any comfort at all.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-10-11 09:05:54 UTC ---
(In reply to comment #6)
> I saw the note that PR/49911 is fixed and thought that might mean this one is
> fixed also.  Unfortunately testing shows that is not the case, at least not in
> 4_5_branch.

Yes, the PR49911 case was a special case where an earlier optimization pass
exposes the issue of this (PR50189) bug.  That exposal is now no longer
happening.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #9 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-10-11 09:09:21 UTC ---
(In reply to comment #7)
> Re comment 5, does "works by luck" mean that I should not look in trunk for a
> fix to backport because nothing was actually fixed?

The bug is quite hard to trigger, minor changes in optimizations or source
can avoid it.  But we didn't yet fix it in any real way.

> Should I just avoid all versions of GCC newer than 4.4?  The verdict "works by
> luck" frankly does not give me any comfort at all.

Ok, change "works by luck in 4.7" to "breaks by unluck in 4.5 and 4.6".  Does
that make you more comfortable?

You can completely avoid the bug by specifying -fno-tree-vrp or using
an optimization level of -O1 or -O0.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[pkoning at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #10 from Paul Koning <pkoning at gcc dot gnu.org> 2011-10-11 19:03:24 UTC ---
Created attachment 25467
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=25467
Tentative patch against 4.6.1

I chased the issue for a while, using 4.6.1 as the test version.

The problem is in extract_range_from_assert.  When processing < <= > >=
assertions, it sets the min (for < <=) or max (for > >=) of the calculated
range to be the type min or max of the right hand side.

In the testcase, we have "m_timestamp > AT_END" where m_timestamp is unsigned
int and AT_END is enum with value 2.  The highest enum value of that enum type
is 3, so if fstrict-enums is in effect, the type max is 3.

Result: while the dump file shows the resulting range as [3,+INF] what that
actually means is [3,3] because the upper bound of the enum is applied, *not*
the upper bound of the variable being compared.

The solution is for extract_range_from_assert to pick up type min or type max
from the type of the left hand side (here m_timestamp, i.e., unsigned int).  So
the range still shows as [3,+INF] but now that represents [3,4294967295] and
the resulting code is correct.

The patch is just one line.  The question I have is whether changing the way
variable "type" is set is right, because it is also used in the != case and I
don't fully understand that one.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |rguenth at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #11 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-10-12 11:42:45 UTC ---
(In reply to comment #10)
> Created attachment 25467 [details]
> Tentative patch against 4.6.1
> 
> I chased the issue for a while, using 4.6.1 as the test version.
> 
> The problem is in extract_range_from_assert.  When processing < <= > >=
> assertions, it sets the min (for < <=) or max (for > >=) of the calculated
> range to be the type min or max of the right hand side.
> 
> In the testcase, we have "m_timestamp > AT_END" where m_timestamp is unsigned
> int and AT_END is enum with value 2.  The highest enum value of that enum type
> is 3, so if fstrict-enums is in effect, the type max is 3.
> 
> Result: while the dump file shows the resulting range as [3,+INF] what that
> actually means is [3,3] because the upper bound of the enum is applied, *not*
> the upper bound of the variable being compared.
> 
> The solution is for extract_range_from_assert to pick up type min or type max
> from the type of the left hand side (here m_timestamp, i.e., unsigned int).  So
> the range still shows as [3,+INF] but now that represents [3,4294967295] and
> the resulting code is correct.
> 
> The patch is just one line.  The question I have is whether changing the way
> variable "type" is set is right, because it is also used in the != case and I
> don't fully understand that one.

While I don't necessarily agree with your conclusion that this is the bug
(GCC expects both operands of the comparisons to be of "compatible types",
and GCC treats types compatible when they have the same precision and
disregards TYPE_{MIN,MAX}_VALUE - which it can't when at the same time
VRP (and _only_ VRP) uses those values for optimization), your patch
makes sense - the type of var is the more natural one to chose (also
when looking at other uses of 'type').  If it mitigates the issue in
more cases that's even better!

I'm going to give it proper testing.

Thanks for spotting this simple solution for your problem.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[pkoning at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #12 from Paul Koning <pkoning at gcc dot gnu.org> 2011-10-12 14:04:30 UTC ---
You said "GCC treats types compatible when they have the same precision". 
That's where the problem lies, because enums with -fstrict-enums have their
precision set to just enough bits to hold the range of values, rather than the
precision of the base integer type.  So in the test case in question, the
variable's precision is 32 bits while the right hand side precision is 2 bits.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #13 from rguenther at suse dot de <rguenther at suse dot de> 2011-10-12 14:15:56 UTC ---
On Wed, 12 Oct 2011, pkoning at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189
> 
> --- Comment #12 from Paul Koning <pkoning at gcc dot gnu.org> 2011-10-12 14:04:30 UTC ---
> You said "GCC treats types compatible when they have the same precision". 
> That's where the problem lies, because enums with -fstrict-enums have their
> precision set to just enough bits to hold the range of values, rather than the
> precision of the base integer type.  So in the test case in question, the
> variable's precision is 32 bits while the right hand side precision is 2 bits.

Well.  At least we lose the conversion then here:

  const unsigned int D.2189;
  position_t D.2188;

<bb 2>:
  D.2189_2 = this_1(D)->m_timestamp;
  D.2188_3 = MIN_EXPR <D.2189_2, 3>;
  return D.2188_3;

I'll look into that.

Richard.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #14 from rguenther at suse dot de <rguenther at suse dot de> 2011-10-12 14:21:48 UTC ---
On Wed, 12 Oct 2011, rguenther at suse dot de wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189
> 
> --- Comment #13 from rguenther at suse dot de <rguenther at suse dot de> 2011-10-12 14:15:56 UTC ---
> On Wed, 12 Oct 2011, pkoning at gcc dot gnu.org wrote:
> 
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189
> > 
> > --- Comment #12 from Paul Koning <pkoning at gcc dot gnu.org> 2011-10-12 14:04:30 UTC ---
> > You said "GCC treats types compatible when they have the same precision". 
> > That's where the problem lies, because enums with -fstrict-enums have their
> > precision set to just enough bits to hold the range of values, rather than the
> > precision of the base integer type.  So in the test case in question, the
> > variable's precision is 32 bits while the right hand side precision is 2 bits.
> 
> Well.  At least we lose the conversion then here:
> 
>   const unsigned int D.2189;
>   position_t D.2188;
> 
> <bb 2>:
>   D.2189_2 = this_1(D)->m_timestamp;
>   D.2188_3 = MIN_EXPR <D.2189_2, 3>;
>   return D.2188_3;
> 
> I'll look into that.

Which is because even with -fstrict-enums the position_t is
unsigned and has precision 32.

 <enumeral_type 0x7ffff5b80540 position_t
    type <integer_type 0x7ffff5b805e8 unsigned int public unsigned SI
        size <integer_cst 0x7ffff7eee2c0 constant 32>
        unit size <integer_cst 0x7ffff7eee2e0 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0x7ffff5b805e8 
precision 2 min <integer_cst 0x7ffff5b89480 0> max <integer_cst 
0x7ffff5b894a0 3>>
    unsigned SI size <integer_cst 0x7ffff7eee2c0 32> unit size 
<integer_cst 0x7ffff7eee2e0 4>
    align 32 symtab 0 alias set -1 canonical type 0x7ffff5b80540 precision 
32 min <integer_cst 0x7ffff5b685a0 0> max <integer_cst 0x7ffff5b894c0 3>

without -fstrict-enums it looks like

 <enumeral_type 0x7ffff5b80540 position_t
    type <integer_type 0x7ffff5b805e8 unsigned int public unsigned SI
        size <integer_cst 0x7ffff7eee2c0 constant 32>
        unit size <integer_cst 0x7ffff7eee2e0 constant 4>
        align 32 symtab 0 alias set -1 canonical type 0x7ffff5b805e8 
precision 2 min <integer_cst 0x7ffff5b89480 0> max <integer_cst 
0x7ffff5b894a0 3>>
    unsigned SI size <integer_cst 0x7ffff7eee2c0 32> unit size 
<integer_cst 0x7ffff7eee2e0 4>
    align 32 symtab 0 alias set -1 canonical type 0x7ffff5b80540 precision 
32 min <integer_cst 0x7ffff7eee300 0> max <integer_cst 0x7ffff7eee2a0 
4294967295>

so it only does not have the TYPE_MIN/MAX_VALUE set to [0,3]

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #15 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-10-12 15:13:04 UTC ---
Author: rguenth
Date: Wed Oct 12 15:12:58 2011
New Revision: 179856

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=179856
Log:
2011-10-12  Paul Koning  <pkoning@gcc.gnu.org>

    PR tree-optimization/50189
    * tree-vrp.c (extract_range_from_assert): Use the type of
    the variable, not the limit.

    * g++.dg/torture/pr50189.C: New testcase.

Added:
    branches/gcc-4_6-branch/gcc/testsuite/g++.dg/torture/pr50189.C
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_6-branch/gcc/tree-vrp.c

[Bug middle-end/50189] [4.5 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |4.6.2
            Summary|[4.5/4.6 Regression] Wrong  |[4.5 Regression] Wrong code
                   |code error in -O2           |error in -O2
                   |[-fstrict-enums] compile,   |[-fstrict-enums] compile,
                   |target independent          |target independent

--- Comment #17 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-10-12 15:16:56 UTC ---
Applied to trunk and the 4.6 branch sofar.

[Bug middle-end/50189] [4.5/4.6 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #16 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-10-12 15:16:17 UTC ---
Author: rguenth
Date: Wed Oct 12 15:16:14 2011
New Revision: 179857

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=179857
Log:
2011-10-12  Paul Koning  <pkoning@gcc.gnu.org>

    PR tree-optimization/50189
    * tree-vrp.c (extract_range_from_assert): Use the type of
    the variable, not the limit.

    * g++.dg/torture/pr50189.C: New testcase.

Added:
    trunk/gcc/testsuite/g++.dg/torture/pr50189.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-vrp.c

[Bug middle-end/50189] [4.5 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #19 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-01-03 15:28:37 UTC ---
Fixed.

[Bug middle-end/50189] [4.5 Regression] Wrong code error in -O2 [-fstrict-enums] compile, target independent

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50189

--- Comment #18 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-01-03 15:28:25 UTC ---
Author: rguenth
Date: Tue Jan  3 15:28:19 2012
New Revision: 182850

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=182850
Log:
2012-01-03  Richard Guenther  <rguenther@suse.de>

    Backport from mainline
    2011-10-12  Paul Koning  <pkoning@gcc.gnu.org>

    PR tree-optimization/50189
    * tree-vrp.c (extract_range_from_assert): Use the type of
    the variable, not the limit.

    * g++.dg/torture/pr50189.C: New testcase.

Added:
    branches/gcc-4_5-branch/gcc/testsuite/g++.dg/torture/pr50189.C
Modified:
    branches/gcc-4_5-branch/gcc/ChangeLog
    branches/gcc-4_5-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_5-branch/gcc/tree-vrp.c