/* The next program is aborted with avr-gcc 4.1.2, 4.2.3, 4.3.0: result of char promotion comes out of CHAR_MIN/MAX. Options: -W -Wall -Os Know to work: 3.3.6, 3.4.6 - good code 4.0.4 - correct, but not the best 4.1.2, 4.2.3, 4.3.0 - without optimization only */ #include <limits.h> void abort (void); void exit (int); void foo (int i) { static int n; if (i < CHAR_MIN || i > CHAR_MAX) abort (); if (++n > 1000) exit (0); } int main () { char c; for (c = 0; ; c++) foo (c); }
Subject: Re: New: [avr] result of char promotion comes out of CHAR_MIN/MAX This code is only defined if char is unsigned which it is not on avr. (It is unsigned on some targets like powerpc-Linux-gnu. Sent from my iPhone On Mar 18, 2008, at 22:42, "dmixm at marine dot febras dot ru" <gcc-bugzilla@gcc.gnu.org > wrote: > /* The next program is aborted with avr-gcc 4.1.2, 4.2.3, 4.3.0: > result of char promotion comes out of CHAR_MIN/MAX. > Options: -W -Wall -Os > Know to work: > 3.3.6, 3.4.6 - good code > 4.0.4 - correct, but not the best > 4.1.2, 4.2.3, 4.3.0 - without optimization only > */ > > #include <limits.h> > > void abort (void); > void exit (int); > > void foo (int i) > { > static int n; > if (i < CHAR_MIN || i > CHAR_MAX) > abort (); > if (++n > 1000) > exit (0); > } > > int main () > { > char c; > for (c = 0; ; c++) foo (c); > } > > > -- > Summary: [avr] result of char promotion comes out of > CHAR_MIN/MAX > Product: gcc > Version: 4.3.0 > Status: UNCONFIRMED > Severity: normal > Priority: P3 > Component: target > AssignedTo: unassigned at gcc dot gnu dot org > ReportedBy: dmixm at marine dot febras dot ru > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35634 >
As I already mentioned this is undefined code, overflow for signed integeral types is undefined. char is a weird type as it is considered one of the character types but it is still an integeral type. Also it is weird that it defualt to either signed or unsigned (implementation defined, in GCC's case it is ABI defined). For AVR, it defaults to signed.
Actually, there is no undefined behaviour here, as long as CHAR_MAX < INT_MAX no overflow occurs. c++ is the same as c = (int)c + 1 (when ignoring the result), and the conversion from int to char is implementation defined. For gcc the result of such a conversion is always in the range of the target type.
This is a bug in the C frontend which does the increment on type char, not on the promoted type (I noticed that while fixing bitfield issues as well), code in question is in build_unary_op() and this way since forever. Original dump as from the FE: ;; Function main (main) ;; enabled by -tree-original { char c; char c; c = 0; <D.1559>:; foo ((int) c); c++ ; goto <D.1559>; } I tried to fix this once but failed. Joseph - can you give this a shot? The FE should for all pre-/postincrements just emit the proper {( int res = x; x = (typeof x)((int)x + 1); res; )} with using TARGET_EXPR/COMPOUND_EXPRs as required. Note the C++ frontend has the same problem here, so transition that bug there once the C FE is fixed. Thanks.
To quote the standard (6.5.4.1/2): "The expression ++E is equivalent to (E+=1). See the discussions of additive operators and compound assignment for information on constraints, types, side effects, and CONVERSIONS and the effects of operations on pointers" emphasise mine, 6.5.6/4 then of course says "If both operands have arithmetic type, the usual arithmetic conversions are performed on them."
Created attachment 15349 [details] Gimplification-time patch Changing at build_unary_op time runs into OpenMP problems - the OpenMP code needs the trees to correspond more directly to the increments and decrements in the source code. Changing at gimplification time, as in the attached patch, avoids that problem, but a number of gcc.dg/vect tests regress because of the changes to the code for increment/decrement of types that get promoted. FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/pr30771.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/vect-multitypes-11.c scan-tree-dump-times vect "vectorized 1 loops" 2 FAIL: gcc.dg/vect/vect-reduc-dot-u16a.c scan-tree-dump-times vect "vectorized 1 loops" 2 FAIL: gcc.dg/vect/slp-21.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/no-scevccp-outer-13.c scan-tree-dump-times vect "OUTER LOOP VECTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-14.c scan-tree-dump-times vect "OUTER LOOP VECTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-16.c scan-tree-dump-times vect "OUTER LOOP VECTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-17.c scan-tree-dump-times vect "OUTER LOOP VECTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-19.c scan-tree-dump-times vect "OUTER LOOP VECTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-21.c scan-tree-dump-times vect "OUTER LOOP VECTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-7.c scan-tree-dump-times vect "OUTER LOOP VECTORIZED." 1
Thanks. I guess the vect fallout needs to be dealt with separately. Now, I think gimplification time is not the best here, can we maybe move this to general gimplification code if we change the {PRE,POST}{IN,DE}CREMENT_EXPR to have the type the increment is done in (and the expression result) be TREE_TYPE of that expression? This way the generic gimplification code would need to make sure to lower it properly. Diego, I suppose this lowering is before tuples come into play and we loose this extra type, right? Of course this may need auditing of the FEs wrt correctness of the type in this expression but feels like a more general fix?
"Now, I think gimplification time is not the best here" Now, if we think ... is the best here obviously ;)
I did a quick scan and Ada, C++ and C ever build these operations. Also a few backends do (mips, rs6000 and s390). So IMHO changing the semantics of these to /* Nodes for ++ and -- in C. The second arg is how much to increment or decrement by. For a pointer, it would be the size of the object pointed to. The type of the expression specifies the type the increment is performed on and the type of the result. This type does not need to match the type of the first argument, instead that is properly size-/zero-extended before the arithmetic operation. */ DEFTREECODE (PREDECREMENT_EXPR, "predecrement_expr", tcc_expression, 2) DEFTREECODE (PREINCREMENT_EXPR, "preincrement_expr", tcc_expression, 2) DEFTREECODE (POSTDECREMENT_EXPR, "postdecrement_expr", tcc_expression, 2) DEFTREECODE (POSTINCREMENT_EXPR, "postincrement_expr", tcc_expression, 2) is reasonable. Note that expansion no longer handles these tree codes, they are expected to only survive until gimplification.
Joseph, do you have that build_unary_op patch still around? If that patch didn't cause any regressions but OpenMP, I could look at tweaking OpenMP...
Created attachment 15382 [details] build_unary_op patch There may well be other regressions with this patch (in particular the vector ones may appear with this patch as well); I stopped testing when the OpenMP failures appeared.
Created attachment 15455 [details] gcc43-pr35634.patch Here is the updated FE only patch. One change is that it avoids P{RE,OST}{IN,DE}CREMENT_EXPR only for the promoting types, and has some (admittedly very ugly) OpenMP parsing changes to counter that. Unfortunately unlike #pragma omp for increment, #pragma omp atomic can have some_lvalue++ , not necessarily a variable_name++, so I have no idea how to handle that. On x86_64 with this patch I get 3 omp failures (2 in libgomp atomic-10.c, one in gcc/testsuite atomic-1.c) and: FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/pr30771.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/vect-multitypes-11.c scan-tree-dump-times vect "vectorized 1 loops" 2 so (depending on where the other patch was tested) doing this in the FE doesn't help much or at all.
Subject: Re: [4.1/4.2/4.3/4.4 Regression] operand of pre-/postin-/decrement not promoted On Wed, 9 Apr 2008, jakub at gcc dot gnu dot org wrote: > ------- Comment #12 from jakub at gcc dot gnu dot org 2008-04-09 14:32 ------- > Created an attachment (id=15455) > --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=15455&action=view) > gcc43-pr35634.patch > > Here is the updated FE only patch. One change is that it avoids > P{RE,OST}{IN,DE}CREMENT_EXPR only for the promoting types, and has some > (admittedly very ugly) OpenMP parsing changes to counter that. Unfortunately > unlike #pragma omp for increment, #pragma omp atomic can have some_lvalue++ > , not necessarily a variable_name++, so I have no idea how to handle that. > > On x86_64 with this patch I get 3 omp failures (2 in libgomp atomic-10.c, one > in gcc/testsuite atomic-1.c) and: > FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1 > FAIL: gcc.dg/vect/pr30771.c scan-tree-dump-times vect "vectorized 1 loops" 1 > FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1 > FAIL: gcc.dg/vect/vect-multitypes-11.c scan-tree-dump-times vect "vectorized 1 > loops" 2 > so (depending on where the other patch was tested) doing this in the FE doesn't > help much or at all. Thanks! I will try doing the P{RE,OST}{IN,DE}CREMENT_EXPR semantic change and handling it in the gimplifier. Just because I am curious how much I break the frontends... After the summit paper deadline is over ;) Richard.
Created attachment 15491 [details] gimple semantics change patch This is the variant I thought about with changing the way types are interpreted for the *CREMENT_EXPRs. The usual problem with vectorizer tests appear as SCEV doesn't handle for example <bb 3>: # i_14 = PHI <i_7(5), 0(2)> D.1560_4 = (int) i_14; a[D.1560_4] = D.1560_4; D.1561_6 = D.1560_4 + 1; i_7 = (short int) D.1561_6; if (i_7 <= 63) goto <bb 5>; else goto <bb 4>; but for correctness reasons we cannot do the increment in signed short int due to the undefined overflow issue. We can avoid the promotion if the result is truncated to an unsigned type (but this is an optimization that I didn't want to put into this patch addressing correctness only). I will re-test this patch, a slightly oder version tested ok apart from the vectorizer fallout.
With the patch in comment #14 we have === g++ tests === Running target unix FAIL: g++.dg/init/bitfield1.C (test for excess errors) FAIL: g++.dg/gomp/atomic-1.C (test for excess errors) FAIL: g++.dg/torture/pr33887-1.C -O0 execution test FAIL: g++.dg/torture/pr33887-1.C -O1 execution test FAIL: g++.dg/torture/pr33887-1.C -O2 execution test FAIL: g++.dg/torture/pr33887-1.C -O3 -fomit-frame-pointer execution test FAIL: g++.dg/torture/pr33887-1.C -O3 -g execution test FAIL: g++.dg/torture/pr33887-1.C -Os execution test === gcc tests === FAIL: gcc.dg/gomp/atomic-1.c (test for excess errors) FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/pr30771.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/vect-multitypes-11.c scan-tree-dump-times vect "vectorized 1 l oops" 2 FAIL: gcc.dg/vect/vect-reduc-dot-u16a.c scan-tree-dump-times vect "vectorized 1 loops" 2 FAIL: gcc.dg/vect/slp-21.c scan-tree-dump-times vect "vectorized 1 loops" 1 FAIL: gcc.dg/vect/no-scevccp-outer-13.c scan-tree-dump-times vect "OUTER LOOP VE CTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-14.c scan-tree-dump-times vect "OUTER LOOP VE CTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-16.c scan-tree-dump-times vect "OUTER LOOP VE CTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-17.c scan-tree-dump-times vect "OUTER LOOP VE CTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-19.c scan-tree-dump-times vect "OUTER LOOP VE CTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-21.c scan-tree-dump-times vect "OUTER LOOP VE CTORIZED." 1 FAIL: gcc.dg/vect/no-scevccp-outer-7.c scan-tree-dump-times vect "OUTER LOOP VEC TORIZED." 1 === libgomp tests === Running target unix FAIL: libgomp.c/atomic-10.c (test for excess errors) WARNING: libgomp.c/atomic-10.c compilation failed to produce executable
Downgrading to P2, the patches so far all seem to be quite risky for the branches, the wrong-code is on a corner case and isn't a recent regression. Regarding the comment #14 patch, I'd say the complete_type should be different from argtype only when !TYPE_UNSIGNED (argtype), for unsigned char or unsigned short the overflow behavior is well defined.
Closing 4.1 branch.
*** Bug 38929 has been marked as a duplicate of this bug. ***
I am going to make this a P1 for 4.5, but it's too late for 4.4.
How much of the fallout (especially the scev-related failures) goes away with -funsafe-loop-optimizations? I'm thinking that it is unavoidable. :-(
Closing 4.2 branch.
On no-undefined-overflow branch the FE can do the increment/decrement on the target type safely (well, there are no NV variants of the {PRE,POST}{IN,DEC}REMENT expressions on the branch, so they at the moment all get lowered to possibly wrapping variants during gimplification). Unfortunately that branch is way from "ready".
Subject: Re: [4.3/4.4/4.5 Regression] operand of pre-/postin-/decrement not promoted On Sat, 11 Apr 2009, rguenth at gcc dot gnu dot org wrote: > On no-undefined-overflow branch the FE can do the increment/decrement on the > target type safely (well, there are no NV variants of the > {PRE,POST}{IN,DEC}REMENT > expressions on the branch, so they at the moment all get lowered to > possibly wrapping variants during gimplification). Of course increment/decrement of signed integer types at least as wide as int should get lowered to the no-overflow variants unless -fwrapv; likewise increment/decrement of pointer types. Whether through a gimplification-time hook or through creating NV variants of increment/decrement and having the front end create those when appropriate.
Subject: Re: [4.3/4.4/4.5 Regression] operand of pre-/postin-/decrement not promoted On Sat, 11 Apr 2009, joseph at codesourcery dot com wrote: > ------- Comment #23 from joseph at codesourcery dot com 2009-04-11 16:30 ------- > Subject: Re: [4.3/4.4/4.5 Regression] operand of pre-/postin-/decrement > not promoted > > On Sat, 11 Apr 2009, rguenth at gcc dot gnu dot org wrote: > > > On no-undefined-overflow branch the FE can do the increment/decrement on the > > target type safely (well, there are no NV variants of the > > {PRE,POST}{IN,DEC}REMENT > > expressions on the branch, so they at the moment all get lowered to > > possibly wrapping variants during gimplification). > > Of course increment/decrement of signed integer types at least as wide as > int should get lowered to the no-overflow variants unless -fwrapv; > likewise increment/decrement of pointer types. Whether through a > gimplification-time hook or through creating NV variants of > increment/decrement and having the front end create those when > appropriate. Indeed. As they are not valid gimple but only in generic I lean to a gimplification-time solution here. Richard.
*** Bug 39736 has been marked as a duplicate of this bug. ***
GCC 4.3.4 is being released, adjusting target milestone.
GCC 4.3.5 is being released, adjusting target milestone.
*** Bug 47937 has been marked as a duplicate of this bug. ***
4.3 branch is being closed, moving to 4.4.7 target.
Any news on this bug?
Well, I'd still go for comment#14 ... we could teach VRP to shorten the operations again, if possible, to avoid the optimization regressions.
4.4 branch is being closed, moving to 4.5.4 target.
The 4.5 branch is being closed, adjusting target milestone.
Bump! I don't want to be impolite, but quoting http://blog.regehr.org/archives/482 "In LLVM/Clang the bug was not known but was fixed in less than 24 hours." Seriously...
Mine.
Created attachment 28794 [details] first patch updated With the first patch updated to apply again and to instead of promoting doing arithmetic in an unsigned type still causes +FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1 +FAIL: gcc.dg/vect/vect-iv-8.c scan-tree-dump-times vect "vectorized 1 loops" 1 +FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1 +FAIL: gcc.dg/vect/pr18536.c -flto scan-tree-dump-times vect "vectorized 1 loop s" 1 +FAIL: gcc.dg/vect/vect-iv-8.c -flto scan-tree-dump-times vect "vectorized 1 lo ops" 1 +FAIL: gcc.dg/vect/vect-iv-8a.c -flto scan-tree-dump-times vect "vectorized 1 l oops" 1 which is better compared to doing the promotion: +FAIL: gcc.dg/vect/pr18536.c scan-tree-dump-times vect "vectorized 1 loops" 1 +FAIL: gcc.dg/vect/vect-iv-8.c scan-tree-dump-times vect "vectorized 1 loops" 1 +FAIL: gcc.dg/vect/vect-iv-8a.c scan-tree-dump-times vect "vectorized 1 loops" 1 +FAIL: gcc.dg/vect/vect-reduc-dot-u16a.c scan-tree-dump-times vect "vectorized 1 loops" 2 +FAIL: gcc.dg/vect/slp-21.c scan-tree-dump-times vect "vectorized 4 loops" 1 +FAIL: gcc.dg/vect/slp-21.c scan-tree-dump-times vect "vectorizing stmts using S LP" 2 +FAIL: gcc.dg/vect/slp-perm-9.c scan-tree-dump-times vect "vectorized 1 loops" 1 +FAIL: gcc.dg/vect/slp-reduc-3.c scan-tree-dump-times vect "vectorized 1 loops" 2 +FAIL: gcc.dg/vect/pr18536.c -flto scan-tree-dump-times vect "vectorized 1 loop s" 1 +FAIL: gcc.dg/vect/vect-iv-8.c -flto scan-tree-dump-times vect "vectorized 1 lo ops" 1 +FAIL: gcc.dg/vect/vect-iv-8a.c -flto scan-tree-dump-times vect "vectorized 1 l oops" 1 +FAIL: gcc.dg/vect/vect-reduc-dot-u16a.c -flto scan-tree-dump-times vect "vecto rized 1 loops" 2 +FAIL: gcc.dg/vect/slp-21.c -flto scan-tree-dump-times vect "vectorized 4 loops " 1 +FAIL: gcc.dg/vect/slp-21.c -flto scan-tree-dump-times vect "vectorizing stmts using SLP" 2 +FAIL: gcc.dg/vect/slp-perm-9.c -flto scan-tree-dump-times vect "vectorized 1 l oops" 1 +FAIL: gcc.dg/vect/slp-reduc-3.c -flto scan-tree-dump-times vect "vectorized 1 loops" 2 +FAIL: gcc.dg/vect/no-scevccp-outer-19.c scan-tree-dump-times vect "OUTER LOOP V ECTORIZED." 1 note that the gimple semantics change patch will play foul with decltype / sizeof ( char += char ) where it likely will result in the promoted type rather than char.
Author: rguenth Date: Wed Nov 28 09:27:10 2012 New Revision: 193882 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=193882 Log: 2012-11-28 Richard Biener <rguenther@suse.de> PR c/35634 * gimple.h (gimplify_self_mod_expr): Declare. * gimplify.c (gimplify_self_mod_expr): Export. Take a different type for performing the arithmetic in. (gimplify_expr): Adjust. * tree-vect-loop-manip.c (vect_can_advance_ivs_p): Strip sign conversions we can re-apply after adjusting the IV. c-family/ * c-gimplify.c (c_gimplify_expr): Gimplify self-modify expressions here and use a type with proper overflow behavior for types that would need to be promoted for the arithmetic. * gcc.dg/torture/pr35634.c: New testcase. * g++.dg/torture/pr35634.C: Likewise. * gcc.dg/vect/pr18536.c: Mark worker function noinline. Added: trunk/gcc/testsuite/g++.dg/torture/pr35634.C trunk/gcc/testsuite/gcc.dg/torture/pr35634.c Modified: trunk/gcc/ChangeLog trunk/gcc/c-family/ChangeLog trunk/gcc/c-family/c-gimplify.c trunk/gcc/gimple.h trunk/gcc/gimplify.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/vect/pr18536.c trunk/gcc/tree-vect-loop-manip.c
Fixed for 4.8.0. Unlikely going to be backported.
GCC 4.6.4 has been released and the branch has been closed.
*** Bug 59162 has been marked as a duplicate of this bug. ***
*** Bug 60229 has been marked as a duplicate of this bug. ***
I will not backport this.
Let's close it then.