Created attachment 25105 [details] 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.
Created attachment 25106 [details] gcc -v output (configure options etc.
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.
Surely related to PR49911. That it works with 4.4, 4.6 and 4.7 is pure luck.
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.
Ah, right. Fails on 4.6.1 with -fstrict-enums as well, but not on trunk where it seems to work by luck.
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.
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.
(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.
(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.
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.
(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.
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.
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.
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]
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
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
Applied to trunk and the 4.6 branch sofar.
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
Fixed.