Hi, The code below produces a warning about returning a reference to a temporary when using enum types for the qMin method. It works fine when using primitive types or classes instead of enums. Fortunately it's rather easy to work around the warning :) Best regards, Lars enum Foo { A, B }; template<typename T> const T &qMin(const T &a, const T &b) { return a < b ? a : b; } int main(int, char **) { Foo f = A; Foo g = B; Foo h = qMin(f, g); return 0; }
Confirmed: <<cleanup_point return <retval> = (const Foo &) (const Foo *) &TARGET_EXPR <D.1580, (const Foo) MIN_EXPR <(int) *b, (int) *a>>>> why is there a cast to int in the MIN_EXPR. This causes wrong code.
Why would this cause wrong code? It's just a cast from an enum to an int, I don't see what's wrong with that except that it's ugly. You have to actually show that this causes wrong code, not just make your usual exaggerated claims ;-)
(In reply to comment #2) > Why would this cause wrong code? It's just a cast from an enum to an int, > I don't see what's wrong with that except that it's ugly. > > You have to actually show that this causes wrong code, not just make your > usual exaggerated claims ;-) Ok, the reason why it is wrong code is because we return an address to a temporary which we can overwrite. Yes this is wrong code. The diagnostic is just an indication that it will be wrong code.
When references are involved, like this, reduction to MAX_EXPR is wrong to begin with. The cast is only an additional symptom.
Subject: Re: [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary "pinskia at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes: | (In reply to comment #2) | > Why would this cause wrong code? It's just a cast from an enum to an int, | > I don't see what's wrong with that except that it's ugly. | > | > You have to actually show that this causes wrong code, not just make your | > usual exaggerated claims ;-) | | Ok, the reason why it is wrong code is because we return an address to a temporary which we can | overwrite. Yes this is wrong code. The diagnostic is just an indication that it will be wrong code. "rth at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes: | When references are involved, like this, reduction to MAX_EXPR is wrong to | begin with. The cast is only an additional symptom. Both Andrew and RTH are obviously right. This is wrong-code generation. As a "proof", I've slighlty modified the testcase to include an assertion on the address of the object that realizes the min. It fails, as can be expected from the warning. Here we go. % cat 19199.C && g++ 19199.C -o 19199 && ./19199 #include <assert.h> enum Foo { A, B }; template<typename T> const T &qMin(const T &a, const T &b) { return a < b ? a : b; } int main(int, char **) { Foo f = A; Foo g = B; const Foo& h = qMin(f, g); assert(&h == &f || &h == &g); return 0; } 19199.C: In function 'const T& qMin(const T&, const T&) [with T = Foo]': 19199.C:12: instantiated from here 19199.C:6: warning: returning reference to temporary 19199: 19199.C:13: int main(int, char**): Assertion `&h == &f || &h == &g' failed. zsh: 10279 abort ./19199 -- Gaby
Hmm, even though 3.1 did not produce the diagnostic we did produce wrong code. So the only part which is a regression is the diagnostic but producing correct code would be very nice.
Roger, this is a middle-end problem in fond_cond_expr_with_comparision; that is the routine which makes the invalid transformation. (It has a similar bug for MAX_EXPR.) If the result of the COND_EXPR is an lvalue, I do not believe this transformation is safe. As we have no language-independent means of testing lvalue-ness, I'm not sure that there's a straightforward fix. Would you care to take a look at this issue?
Actually, I have a standards question here. Assume for the purposes of discussion here that a source-level reference variable "X" is represented as a pointer variable "x" in the intermediate language. E.g. int &A, &B, C; C = A + B; lowers to int *a, *b, C; C = *a + *b; It appears that [expr.cond]/3 *does* preserve references. Thus it would seem that the front end should lower int &A, &B, &C; C = (A < B ? A : B); to int *a, *b, C; c = (*a < *b ? a : b); and not int *a, *b, *c; c = &(*a < *b ? *a : *b); Note that comment #1 indicates that we're emitting the later. If we'd actually written (*a < *b ? *a : *b) at the source level in C, it would be illegal for us to take the address of the result, and so the transformation to MAX_EXPR would be legitimate. But I'm not sure what the actual rules for expressions for which references are legitimate in C++. If it's legitimate to write int A, &B, &C; C = (A < B ? A : B); -> int A, *b, *c; c = &(A < *b ? A : *b); then I think perhaps a more accurate representation would be int A, *b, *c, *t; c = &*((A < *b ? t = &A : t = &*b), t);
Is the folowing the same bug? extern void abort (void); int i0 = 999; int *const p = &i0; int const *const & foo () { return p; } int main () { int i = *foo (); if (i != i0) abort (); return 0; } This warns, and fails at -O1. At -O0 the code is also wrong, but it passes because the assignment to the stack temporary happens, and the stack is not scribbled over. If I make the creation of a const * const & temporary explicit, the warning is not emitted, but the code is still wrong in the same way: extern void abort (void); int i0 = 999; int *const p = &i0; int const *const & foo () { int const *const &p0 = p; return p0; } int main () { int i = *foo (); if (i != i0) abort (); return 0; } If I define p instead with int const *const p = &i0; no bogus temporary is generated.
(In reply to comment #9) Hmmm, closer inspection of the standard seems to indicate that this is what the standard says should happen. I think that is a defect, though, propably best addressed together with DR 450.
(Of course, the root cause of this problem is that "fold" is being called before gimplification, which Nathan and I have sermonized about previously.) There's interesting interplay between this PR and PR 20280. In particular, RTH's proposed transformation in Comment #8 is invalid if A and B are bit-fields, because the address of a bitfield cannot be taken. Consider: struct S { int i : 7; }; S s; extern int &b; const int &a = (x ? s.i : b); This cannot be transformed into: int *a = &*((x ? t = &s.i : t = &*b), t); as per Comment #8. Indeed fold transforms ((s.i > i)? s.i : i) into MAX_EXPR (s.i, i) in this program: struct S { int i : 7; }; S s; extern int &i; bool b; void g() { ((s.i > i) ? s.i : i) = 3; } That happens to work only because of the hack where we allow MAX_EXPR as an lvalue in an assignment in build_modify_expr if neither argument has side-effects. (We recreate the COND_EXPR there.) This change went in as the fix for PR 7503. Roger, I think you need the same hack where you bind COND_EXPRs to references, take their addresses, etc. In fact, you should probably just make real_lvalue_p accept MAX_EXPRs with side-effect free operands. And, then, wherever we need lvalues do the conversion that you presently do in build_modify_expr. That's consistent with your "MAX_EXPRs are just the canonical form of certain COND_EXPRs" logic.
Subject: [PR c++/19199] don't turn cond_expr lvalue into min_expr rvalue (continued from PR c++/20280) On Mar 4, 2005, Mark Mitchell <mark@codesourcery.com> wrote: > Actually, looking at this more closely, I think that something is > forgetting to call rationalize_conditional_expr, which is normally > called from unary_complex_lvalue. When the conditional expression is > used in the lvalue context, something should be calling that. > Normally, that happens because something calls build_unary_op > (ADDR_EXPR, ...) on the COND_EXPR. > It may be that I changed something to call build_addr (instead of > build_unary_op) in a case where that's not safe. Can you confirm or > deny that hypothesis? I'm not sure you're still talking about PR 20280, or about PR 19199, that is marked as a blocker because it appears to be the same problem, but AFAICT really isn't. Here's a patch that fixes PR c++/19199, by avoiding the transformation from: <cond <lt <nop(int) <parm a> > <nop(int) <parm b> > > <parm a> <parm b> > into: <nop(enum) <min <nop(int) <parm a> > <nop(int) <parm b> > > since the latter is clearly not an lvalue, and rationalize_conditional_expr doesn't manage to turn it back into one (I don't think it should). I realize we might miss some optimization opportunities because of this change in C, because the optimization would be valid there, but I suppose we could arrange for cond_expr operands in C to be forced into rvalues. Still testing on x86_64-linux-gnu. Ok to install if it passes? Index: gcc/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * fold-const.c (non_lvalue): Split tests into... (maybe_lvalue_p): New function. (fold_ternary): Use it to avoid turning a COND_EXPR lvalue into an MIN_EXPR rvalue. Index: gcc/fold-const.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v retrieving revision 1.523 diff -u -p -r1.523 fold-const.c --- gcc/fold-const.c 4 Mar 2005 06:24:09 -0000 1.523 +++ gcc/fold-const.c 4 Mar 2005 19:04:26 -0000 @@ -2004,16 +2004,12 @@ fold_convert (tree type, tree arg) } } -/* Return an expr equal to X but certainly not valid as an lvalue. */ +/* Return false if expr can be assumed to not be an lvalue, true + otherwise. */ -tree -non_lvalue (tree x) +static bool +maybe_lvalue_p (tree x) { - /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to - us. */ - if (in_gimple_form) - return x; - /* We only need to wrap lvalue tree codes. */ switch (TREE_CODE (x)) { @@ -2053,8 +2049,24 @@ non_lvalue (tree x) /* Assume the worst for front-end tree codes. */ if ((int)TREE_CODE (x) >= NUM_TREE_CODES) break; - return x; + return false; } + + return true; +} + +/* Return an expr equal to X but certainly not valid as an lvalue. */ + +tree +non_lvalue (tree x) +{ + /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to + us. */ + if (in_gimple_form) + return x; + + if (! maybe_lvalue_p (x)) + return x; return build1 (NON_LVALUE_EXPR, TREE_TYPE (x), x); } @@ -7095,10 +7107,17 @@ fold_ternary (tree expr) of B and C. Signed zeros prevent all of these transformations, for reasons given above each one. + We don't want to use operand_equal_for_comparison_p here, + because it might turn an lvalue COND_EXPR into an rvalue one, + see PR c++/19199. + Also try swapping the arguments and inverting the conditional. */ if (COMPARISON_CLASS_P (arg0) - && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), - arg1, TREE_OPERAND (arg0, 1)) + && ((maybe_lvalue_p (arg1) + && maybe_lvalue_p (TREE_OPERAND (t, 2))) + ? operand_equal_p (TREE_OPERAND (arg0, 0), arg1, OEP_ONLY_CONST) + : operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), + arg1, TREE_OPERAND (arg0, 1))) && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (arg1)))) { tem = fold_cond_expr_with_comparison (type, arg0, @@ -7109,9 +7128,13 @@ fold_ternary (tree expr) } if (COMPARISON_CLASS_P (arg0) - && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), - TREE_OPERAND (t, 2), - TREE_OPERAND (arg0, 1)) + && ((maybe_lvalue_p (arg1) + && maybe_lvalue_p (TREE_OPERAND (t, 2))) + ? operand_equal_p (TREE_OPERAND (arg0, 0), + TREE_OPERAND (t, 2), OEP_ONLY_CONST) + : operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), + TREE_OPERAND (t, 2), + TREE_OPERAND (arg0, 1))) && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (TREE_OPERAND (t, 2))))) { tem = invert_truthvalue (arg0); Index: gcc/testsuite/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * g++.dg/expr/lval2.C: New. Index: gcc/testsuite/g++.dg/expr/lval2.C =================================================================== RCS file: gcc/testsuite/g++.dg/expr/lval2.C diff -N gcc/testsuite/g++.dg/expr/lval2.C --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ gcc/testsuite/g++.dg/expr/lval2.C 4 Mar 2005 19:04:42 -0000 @@ -0,0 +1,26 @@ +// PR c++/19199 + +// { dg-do run } + +// We used to turn the COND_EXPR lvalue into a MIN_EXPR rvalue, and +// then return a reference to a temporary in qMin. + +#include <assert.h> + +enum Foo { A, B }; + +template<typename T> T &qMin(T &a, T &b) +{ + return a < b ? a : b; +} + +int main (int, char **) +{ + Foo f = A; + Foo g = B; + Foo &h = qMin(f, g); + assert (&h == &f || &h == &g); + const Foo &i = qMin((const Foo&)f, (const Foo&)g); + assert (&i == &f || &i == &g); + return 0; +} -- Alexandre Oliva http://www.ic.unicamp.br/~oliva/ Red Hat Compiler Engineer aoliva@{redhat.com, gcc.gnu.org} Free Software Evangelist oliva@{lsd.ic.unicamp.br, gnu.org}
Subject: Re: [PR c++/19199] don't turn cond_expr lvalue into min_expr rvalue (continued from PR c++/20280) Alexandre Oliva wrote: > Here's a patch that fixes PR c++/19199, by avoiding the transformation > from: > > <cond <lt <nop(int) <parm a> > <nop(int) <parm b> > > > <parm a> <parm b> > > > into: > > <nop(enum) <min <nop(int) <parm a> > <nop(int) <parm b> > > > > since the latter is clearly not an lvalue, and > rationalize_conditional_expr doesn't manage to turn it back into one > (I don't think it should). I realize we might miss some optimization > opportunities because of this change in C, because the optimization > would be valid there, but I suppose we could arrange for cond_expr > operands in C to be forced into rvalues. Roger has objected to this change in the past. As I noted in the audit trail for 19199, there's stuff in build_modify_expr to handle MIN_EXPR/MAX_EXPR as lvalues -- but, if Roger wants to stick with that approach, it needs to be spread throughout the C++ front end. I'd be happier with your approach overall, in part because I think fold is doing too much, too early, in general, but I don't feel comfortable approving the patch.
Subject: Re: [PR c++/19199] don't turn cond_expr lvalue into min_expr rvalue (continued from PR c++/20280) On Mar 5, 2005, Mark Mitchell <mark@codesourcery.com> wrote: > Roger has objected to this change in the past. As I noted in the > audit trail for 19199, there's stuff in build_modify_expr to handle > MIN_EXPR/MAX_EXPR as lvalues Problem is, with the transformation performed by fold, it's no longer an lvalue, and forcing it back into an lvalue just because it looks like it should be one could break other programs.
Subject: Re: [PR c++/19199] don't turn cond_expr lvalue into min_expr rvalue (continued from PR c++/20280) Alexandre Oliva wrote: > On Mar 5, 2005, Mark Mitchell <mark@codesourcery.com> wrote: > > >>Roger has objected to this change in the past. As I noted in the >>audit trail for 19199, there's stuff in build_modify_expr to handle >>MIN_EXPR/MAX_EXPR as lvalues > > > Problem is, with the transformation performed by fold, it's no longer > an lvalue, and forcing it back into an lvalue just because it looks > like it should be one could break other programs. Yes, I understand. You still need to take it up with Roger, though.
Subject: Re: [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary On Mar 7, 2005, Roger Sayle <roger@eyesopen.com> wrote: > For example, I believe that Alex's proposed solution to PR c++/19199 > isn't an appropriate fix. It's perfectly reasonable for fold to convert > a C++ COND_EXPR into a MIN_EXPR or MAX_EXPR, as according to the C++ > front-end all three of these tree nodes are valid lvalues. Hence it's > not this transformation in fold that's in error. Bugzilla was kept out of the long discussion that ensued, so I'll try to summarize. The problem is that the conversion is turning a COND_EXPR such as: ((int)a < (int)b ? a : b) into (__typeof(a)) ((int)a <? (int)b) which is not an lvalue. This is the transformation we have to avoid. > Simply disabling the COND_EXPR -> MIN_EXPR/MAX_EXPR transformation is > also likely to be a serious performance penalty, especially on targets > that support efficient sequences for "min" and "max". This was not what I intended to do with my patch, FWIW. Unfortunately, I goofed in the added call to operand_equal_p, limiting too much the situations in which the optimization could be applied. The patch fixes this problem, and updates the patch such that it applies cleanly again. As for other languages whose COND_EXPRs can't be lvalues, we can probably arrange quite easily for them to ensure at least one of their result operands is not an lvalue, so as to enable the transformation again. Comments? Ok to install? Index: gcc/ChangeLog from Alexandre Oliva <aoliva@redhat.com> * fold-const.c (non_lvalue): Split tests into... (maybe_lvalue_p): New function. (fold_ternary): Use it to avoid turning a COND_EXPR lvalue into a MIN_EXPR rvalue. Index: gcc/fold-const.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v retrieving revision 1.535 diff -u -p -r1.535 fold-const.c --- gcc/fold-const.c 7 Mar 2005 21:24:21 -0000 1.535 +++ gcc/fold-const.c 8 Mar 2005 22:07:52 -0000 @@ -2005,16 +2005,12 @@ fold_convert (tree type, tree arg) } } -/* Return an expr equal to X but certainly not valid as an lvalue. */ +/* Return false if expr can be assumed to not be an lvalue, true + otherwise. */ -tree -non_lvalue (tree x) +static bool +maybe_lvalue_p (tree x) { - /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to - us. */ - if (in_gimple_form) - return x; - /* We only need to wrap lvalue tree codes. */ switch (TREE_CODE (x)) { @@ -2054,8 +2050,24 @@ non_lvalue (tree x) /* Assume the worst for front-end tree codes. */ if ((int)TREE_CODE (x) >= NUM_TREE_CODES) break; - return x; + return false; } + + return true; +} + +/* Return an expr equal to X but certainly not valid as an lvalue. */ + +tree +non_lvalue (tree x) +{ + /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to + us. */ + if (in_gimple_form) + return x; + + if (! maybe_lvalue_p (x)) + return x; return build1 (NON_LVALUE_EXPR, TREE_TYPE (x), x); } @@ -9734,10 +9746,16 @@ fold_ternary (tree expr) of B and C. Signed zeros prevent all of these transformations, for reasons given above each one. + We don't want to use operand_equal_for_comparison_p here, + because it might turn an lvalue COND_EXPR into an rvalue one, + see PR c++/19199. + Also try swapping the arguments and inverting the conditional. */ if (COMPARISON_CLASS_P (arg0) - && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), - arg1, TREE_OPERAND (arg0, 1)) + && ((maybe_lvalue_p (op1) && maybe_lvalue_p (op2)) + ? operand_equal_p (TREE_OPERAND (arg0, 0), op1, 0) + : operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), + arg1, TREE_OPERAND (arg0, 1))) && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (arg1)))) { tem = fold_cond_expr_with_comparison (type, arg0, op1, op2); @@ -9746,9 +9764,10 @@ fold_ternary (tree expr) } if (COMPARISON_CLASS_P (arg0) - && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), - op2, - TREE_OPERAND (arg0, 1)) + && ((maybe_lvalue_p (op1) && maybe_lvalue_p (op2)) + ? operand_equal_p (TREE_OPERAND (arg0, 0), op2, 0) + : operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), + op2, TREE_OPERAND (arg0, 1))) && !HONOR_SIGNED_ZEROS (TYPE_MODE (TREE_TYPE (op2)))) { tem = invert_truthvalue (arg0); Index: gcc/testsuite/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * g++.dg/expr/lval2.C: New. Index: gcc/testsuite/g++.dg/expr/lval2.C =================================================================== RCS file: gcc/testsuite/g++.dg/expr/lval2.C diff -N gcc/testsuite/g++.dg/expr/lval2.C --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ gcc/testsuite/g++.dg/expr/lval2.C 8 Mar 2005 22:08:07 -0000 @@ -0,0 +1,26 @@ +// PR c++/19199 + +// { dg-do run } + +// We used to turn the COND_EXPR lvalue into a MIN_EXPR rvalue, and +// then return a reference to a temporary in qMin. + +#include <assert.h> + +enum Foo { A, B }; + +template<typename T> T &qMin(T &a, T &b) +{ + return a < b ? a : b; +} + +int main (int, char **) +{ + Foo f = A; + Foo g = B; + Foo &h = qMin(f, g); + assert (&h == &f || &h == &g); + const Foo &i = qMin((const Foo&)f, (const Foo&)g); + assert (&i == &f || &i == &g); + return 0; +} Index: gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * expr2.C: Fixed. Index: gcc/testsuite/g++.old-deja/g++.oliva/expr2.C =================================================================== RCS file: /cvs/gcc/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C,v retrieving revision 1.5 diff -u -p -r1.5 expr2.C --- gcc/testsuite/g++.old-deja/g++.oliva/expr2.C 1 May 2003 02:02:47 -0000 1.5 +++ gcc/testsuite/g++.old-deja/g++.oliva/expr2.C 8 Mar 2005 22:08:07 -0000 @@ -1,4 +1,4 @@ -// { dg-do run { xfail *-*-* } } +// { dg-do run } // Copyright (C) 2000 Free Software Foundation -- Alexandre Oliva http://www.ic.unicamp.br/~oliva/ Red Hat Compiler Engineer aoliva@{redhat.com, gcc.gnu.org} Free Software Evangelist oliva@{lsd.ic.unicamp.br, gnu.org}
Subject: Re: [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary On 8 Mar 2005, Alexandre Oliva wrote: > > * fold-const.c (non_lvalue): Split tests into... > (maybe_lvalue_p): New function. > (fold_ternary): Use it to avoid turning a COND_EXPR lvalue into > a MIN_EXPR rvalue. This version is Ok for mainline, and currently open release branches. Thanks, Roger --
Subject: Re: [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary On Mar 8, 2005, Roger Sayle <roger@eyesopen.com> wrote: > On 8 Mar 2005, Alexandre Oliva wrote: >> >> * fold-const.c (non_lvalue): Split tests into... >> (maybe_lvalue_p): New function. >> (fold_ternary): Use it to avoid turning a COND_EXPR lvalue into >> a MIN_EXPR rvalue. > This version is Ok for mainline, and currently open release branches. Unfortunately, the problem in g++.oliva/expr2.C resurfaced, because, as it turns out, the transformation (I != J ? I : J) => I yields an lvalue as required, but not the *correct* lvalue in all cases. I tried what you'd suggested on IRC (testing whether the thing is an lvalue after the fact), but that obviously failed in this case as well. So I figured a better approach would be to perform lvalue tests to fold_cond_expr_with_comparison(), where we can avoid specific inappropriate transformations while still trying other alternatives. While at that, I figured we could use pedantic_lvalues to avoid refraining from applying optimizations just because it looks like the cond-expr might be an lvalue, even though the language requires it not to be. This is currently bootstrapping on x86_64-linux-gnu. Ok to install if bootstrap completes and regtesting passes? Index: gcc/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * fold-const.c (non_lvalue): Split tests into... (maybe_lvalue_p): New function. (fold_cond_expr_with_comparison): Preserve lvalue-ness. Index: gcc/fold-const.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v retrieving revision 1.535 diff -u -p -r1.535 fold-const.c --- gcc/fold-const.c 7 Mar 2005 21:24:21 -0000 1.535 +++ gcc/fold-const.c 9 Mar 2005 03:59:18 -0000 @@ -2005,16 +2005,12 @@ fold_convert (tree type, tree arg) } } -/* Return an expr equal to X but certainly not valid as an lvalue. */ +/* Return false if expr can be assumed to not be an lvalue, true + otherwise. */ -tree -non_lvalue (tree x) +static bool +maybe_lvalue_p (tree x) { - /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to - us. */ - if (in_gimple_form) - return x; - /* We only need to wrap lvalue tree codes. */ switch (TREE_CODE (x)) { @@ -2054,8 +2050,24 @@ non_lvalue (tree x) /* Assume the worst for front-end tree codes. */ if ((int)TREE_CODE (x) >= NUM_TREE_CODES) break; - return x; + return false; } + + return true; +} + +/* Return an expr equal to X but certainly not valid as an lvalue. */ + +tree +non_lvalue (tree x) +{ + /* While we are in GIMPLE, NON_LVALUE_EXPR doesn't mean anything to + us. */ + if (in_gimple_form) + return x; + + if (! maybe_lvalue_p (x)) + return x; return build1 (NON_LVALUE_EXPR, TREE_TYPE (x), x); } @@ -4162,7 +4174,15 @@ fold_cond_expr_with_comparison (tree typ tree arg00 = TREE_OPERAND (arg0, 0); tree arg01 = TREE_OPERAND (arg0, 1); tree arg1_type = TREE_TYPE (arg1); - tree tem; + tree tem = NULL; + /* If the COND_EXPR can possibly be an lvalue, we don't want to + perform transformations that return a simplified result that will + be recognized as lvalue, but that will not match the expected + result. We may still return other expressions that would be + incorrect, but those are going to be rvalues, and the caller is + supposed to discard them. */ + bool lvalue = !pedantic_lvalues + && maybe_lvalue_p (arg1) && maybe_lvalue_p (arg2); STRIP_NOPS (arg1); STRIP_NOPS (arg2); @@ -4196,10 +4216,12 @@ fold_cond_expr_with_comparison (tree typ case EQ_EXPR: case UNEQ_EXPR: tem = fold_convert (arg1_type, arg1); - return pedantic_non_lvalue (fold_convert (type, negate_expr (tem))); + tem = pedantic_non_lvalue (fold_convert (type, negate_expr (tem))); + break; case NE_EXPR: case LTGT_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg1)); + tem = pedantic_non_lvalue (fold_convert (type, arg1)); + break; case UNGE_EXPR: case UNGT_EXPR: if (flag_trapping_math) @@ -4211,7 +4233,8 @@ fold_cond_expr_with_comparison (tree typ arg1 = fold_convert (lang_hooks.types.signed_type (TREE_TYPE (arg1)), arg1); tem = fold (build1 (ABS_EXPR, TREE_TYPE (arg1), arg1)); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); + break; case UNLE_EXPR: case UNLT_EXPR: if (flag_trapping_math) @@ -4222,12 +4245,18 @@ fold_cond_expr_with_comparison (tree typ arg1 = fold_convert (lang_hooks.types.signed_type (TREE_TYPE (arg1)), arg1); tem = fold (build1 (ABS_EXPR, TREE_TYPE (arg1), arg1)); - return negate_expr (fold_convert (type, tem)); + tem = negate_expr (fold_convert (type, tem)); + break; default: gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison); break; } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* A != 0 ? A : 0 is simply A, unless A is -0. Likewise A == 0 ? A : 0 is always 0 unless A is -0. Note that both transformations are correct when A is NaN: A != 0 @@ -4236,11 +4265,16 @@ fold_cond_expr_with_comparison (tree typ if (integer_zerop (arg01) && integer_zerop (arg2)) { if (comp_code == NE_EXPR) - return pedantic_non_lvalue (fold_convert (type, arg1)); + tem = pedantic_non_lvalue (fold_convert (type, arg1)); else if (comp_code == EQ_EXPR) - return fold_convert (type, integer_zero_node); + tem = fold_convert (type, integer_zero_node); } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* Try some transformations of A op B ? A : B. A == B? A : B same as B @@ -4284,9 +4318,15 @@ fold_cond_expr_with_comparison (tree typ switch (comp_code) { case EQ_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg2)); + if (lvalue) + break; + tem = pedantic_non_lvalue (fold_convert (type, arg2)); + break; case NE_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg1)); + if (lvalue) + break; + tem = pedantic_non_lvalue (fold_convert (type, arg1)); + break; case LE_EXPR: case LT_EXPR: case UNLE_EXPR: @@ -4302,7 +4342,7 @@ fold_cond_expr_with_comparison (tree typ tem = (comp_code == LE_EXPR || comp_code == UNLE_EXPR) ? fold (build2 (MIN_EXPR, comp_type, comp_op0, comp_op1)) : fold (build2 (MIN_EXPR, comp_type, comp_op1, comp_op0)); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); } break; case GE_EXPR: @@ -4316,16 +4356,16 @@ fold_cond_expr_with_comparison (tree typ tem = (comp_code == GE_EXPR || comp_code == UNGE_EXPR) ? fold (build2 (MAX_EXPR, comp_type, comp_op0, comp_op1)) : fold (build2 (MAX_EXPR, comp_type, comp_op1, comp_op0)); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); } break; case UNEQ_EXPR: - if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) - return pedantic_non_lvalue (fold_convert (type, arg2)); + if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) + tem = pedantic_non_lvalue (fold_convert (type, arg2)); break; case LTGT_EXPR: - if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) - return pedantic_non_lvalue (fold_convert (type, arg1)); + if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) + tem = pedantic_non_lvalue (fold_convert (type, arg1)); break; default: gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison); @@ -4333,6 +4373,11 @@ fold_cond_expr_with_comparison (tree typ } } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* If this is A op C1 ? A : C2 with C1 and C2 constant integers, we might still be able to simplify this. For example, if C1 is one less or one more than C2, this might have started @@ -4347,7 +4392,7 @@ fold_cond_expr_with_comparison (tree typ case EQ_EXPR: /* We can replace A with C1 in this case. */ arg1 = fold_convert (type, arg01); - return fold (build3 (COND_EXPR, type, arg0, arg1, arg2)); + tem = fold (build3 (COND_EXPR, type, arg0, arg1, arg2)); case LT_EXPR: /* If C1 is C2 + 1, this is min(A, C2). */ @@ -4357,8 +4402,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (PLUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold (build2 (MIN_EXPR, - type, arg1, arg2))); + tem = pedantic_non_lvalue (fold (build2 (MIN_EXPR, + type, arg1, arg2))); break; case LE_EXPR: @@ -4369,8 +4414,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (MINUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold (build2 (MIN_EXPR, - type, arg1, arg2))); + tem = pedantic_non_lvalue (fold (build2 (MIN_EXPR, + type, arg1, arg2))); break; case GT_EXPR: @@ -4381,8 +4426,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (MINUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold (build2 (MAX_EXPR, - type, arg1, arg2))); + tem = pedantic_non_lvalue (fold (build2 (MAX_EXPR, + type, arg1, arg2))); break; case GE_EXPR: @@ -4393,8 +4438,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (PLUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold (build2 (MAX_EXPR, - type, arg1, arg2))); + tem = pedantic_non_lvalue (fold (build2 (MAX_EXPR, + type, arg1, arg2))); break; case NE_EXPR: break; @@ -4402,6 +4447,11 @@ fold_cond_expr_with_comparison (tree typ gcc_unreachable (); } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + return NULL_TREE; } Index: gcc/testsuite/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * g++.dg/expr/lval2.C: New. Index: gcc/testsuite/g++.dg/expr/lval2.C =================================================================== RCS file: gcc/testsuite/g++.dg/expr/lval2.C diff -N gcc/testsuite/g++.dg/expr/lval2.C --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ gcc/testsuite/g++.dg/expr/lval2.C 9 Mar 2005 03:59:32 -0000 @@ -0,0 +1,26 @@ +// PR c++/19199 + +// { dg-do run } + +// We used to turn the COND_EXPR lvalue into a MIN_EXPR rvalue, and +// then return a reference to a temporary in qMin. + +#include <assert.h> + +enum Foo { A, B }; + +template<typename T> T &qMin(T &a, T &b) +{ + return a < b ? a : b; +} + +int main (int, char **) +{ + Foo f = A; + Foo g = B; + Foo &h = qMin(f, g); + assert (&h == &f || &h == &g); + const Foo &i = qMin((const Foo&)f, (const Foo&)g); + assert (&i == &f || &i == &g); + return 0; +} Index: gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * expr2.C: Fixed. Index: gcc/testsuite/g++.old-deja/g++.oliva/expr2.C =================================================================== RCS file: /cvs/gcc/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C,v retrieving revision 1.5 diff -u -p -r1.5 expr2.C --- gcc/testsuite/g++.old-deja/g++.oliva/expr2.C 1 May 2003 02:02:47 -0000 1.5 +++ gcc/testsuite/g++.old-deja/g++.oliva/expr2.C 9 Mar 2005 03:59:32 -0000 @@ -1,4 +1,4 @@ -// { dg-do run { xfail *-*-* } } +// { dg-do run } // Copyright (C) 2000 Free Software Foundation -- Alexandre Oliva http://www.ic.unicamp.br/~oliva/ Red Hat Compiler Engineer aoliva@{redhat.com, gcc.gnu.org} Free Software Evangelist oliva@{lsd.ic.unicamp.br, gnu.org}
Splitting non_value into maybe_lvalue_p is a good thing, is totally safe and is preapproved for both mainline and the 4.0 branch. The remaining change to fold_ternary/fold_cond_expr_with_comparison are more controversial, and can theoretically be discussed independently. Reading through each of the transformations of COND_EXPR in fold, all of the problematic transformations are guarded in the block beginning at line 4268 of fold-const.c. This is the set of "A op B ? A : B" transformations. All other transformations are either lvalue-safe, or require either operand 2 or operand 3 to be a non-lvalue (typically operand 3 must be a constant). I believe a suitable 4.0-timescale (grotesque hack workaround) is (untested): --- 4265,4275 ---- a number and A is not. The conditions in the original expressions will be false, so all four give B. The min() and max() versions would give a NaN instead. */ ! if (operand_equal_for_comparison_p (arg01, arg2, arg00) ! && (in_gimple_form ! || strcmp (lang_hooks.name, "GNU C++") != 0 ! || ! maybe_lvalue_p (arg1) ! || ! maybe_lvalue_p (arg2))) { tree comp_op0 = arg00; tree comp_op1 = arg01; The maybe_lvalue_p tests should be obvious from the previous versions of Alexandre's patch. The remaining two lines are both hideous hacks. The first is that these transformations only need to be disabled for the C++ front-end. This is (AFAIK) the only language front-end that uses COND_EXPRs as lvalues, and disabling "x > y ? x : y" into MAX_EXPR (where x and y are VAR_DECLs) is less than ideal for the remaining front-ends. The second equally bad hack is to re-allow this transformation even for C++ once we're in the tree-ssa optimizers. I believe once we're in gimple, COND_EXPR is no longer allowed as the lhs of an assignment, hence the MAX_EXPR/MIN_EXPR recognition transformations are the gimple-level should be able to clean-up any rvalue COND_EXPRs they're presented with. Clearly, testing lang_hooks.name is an option of last resort. I'm increasingly convinced that the correct long term solution is to introduce a new LCOND_EXPR tree node for use by the C++ end. Either as a C++-only tree code, or a generic tree code. Additionally, depending upon whether we go ahead and deprecate >?= and <?= operators, we may or may not also require LMIN_EXPR and LMAX_EXPR. This allows us to correctly separate the semantics: MIN_EXPR is commutative, LMIN_EXPR isn't, MIN_EXPR is not an lvalue, LMIN_EXPR is. If either operand of a L*_EXPR is not an lvalue, it can be folded to the rvalue form. The problematic transformations above can then be controlled via COND_EXPR vs. LCOND_EXPR rather than checking the front-end. cp/call.c's build_conditional_expr is then tweaked to use LCOND_EXPR instead of COND_EXPR. Finally either with tweaks to the grammar, gimplification or g++'s other tree manipulation machinery we can introduce a lower_to_rvalue call to recursively convert LCOND_EXPR into COND_EXPR, as soon as we know the expression can't be used as an lvalue. [Aside: In fact, a fold_rvalue could be used by the middle-end, to additionally handle cases such as const_array[0] which can't currently be handled due to the fear of being used in an lvalue-context, such as &(const_array[0]). Whilst I see LCOND_EXPR (and maybe LMIN_EXPR, LMAX_EXPR) as the long-term way to go, this is unquestionably too disruptive for the gcc-4_0-branch. This leaves us with two options, either suffer an rare miscompilation or take a performance hit in the sake of correctness. I'm in favour of the latter provided we don't unfairly impact the C/fortran/ada and Java front-ends that aren't otherwise affected. The above changes even attempt to allow the std::min and std::max operators to continue to be optimized, in an attempt to limit the performance impact to pre-gimple tree-folding during C++ parsing. Alex, could you confirm that the above suggestion resolves the PR when used in combination with your maybe_lvalue_p split? Mark, do you agree that this is a reasonable (the most reasonable?) compromise in the 4.0 timeframe? I'm not proud of querying the front-end in fold-const.c, but the semantics required of COND_EXPR by the C++ front-end conflict with those of the other front-ends. Normally this means different tree codes, but that's a luxury I don't think we can currently afford.
Subject: Re: [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary > Alex, could you confirm that the above suggestion resolves the PR when used > in combination with your maybe_lvalue_p split? Mark, do you agree that this > is a reasonable (the most reasonable?) compromise in the 4.0 timeframe? Thank you for looking at this again. Yes, I think your patch is a reasonable solution in the 4.0 timeframe. It does, of course, mean that there will be programs that are less well-optimized in C++ than in C, but I agree with you that there's no good way around that in the short term. If you're worried about the cost of the strcmp, you could cache the result, but I have no reason to think that's a real problem. Also, I think this patch should go on the mainline too, until we get a better fix -- but I think we should work on that, so that this doesn't actually make it into 4.1. (In 4.1, I disagree that we need new tree codes. I think all we need to do is wait to call fold until gimplification time in C++; then, the middle end will never see COND_EXPRs used as lvalues. In C++, conditionals can be lvalues, but we actually fix that up before passing them to the middle end. The middle end sees COND_EXPRs that would be lvalues in C++, but they are only used as rvalues, so the transformation in fold would be fine. But, we can have that debate later!) Thanks,
Excuse me for asking, but what is it that makes the latest patch I posted not reasonable for the 4.0 timeframe?
Subject: Re: [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary aoliva at gcc dot gnu dot org wrote: > ------- Additional Comments From aoliva at gcc dot gnu dot org 2005-03-30 02:55 ------- > Excuse me for asking, but what is it that makes the latest patch I posted not > reasonable for the 4.0 timeframe? I'm not sure if there is anything that makes it unreasonable; I don't know either way. I hope Roger will comment.
Subject: Re: [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary On Mar 9, 2005, Alexandre Oliva <aoliva@redhat.com> wrote: > PR c++/19199 > * fold-const.c (non_lvalue): Split tests into... > (maybe_lvalue_p): New function. > (fold_cond_expr_with_comparison): Preserve lvalue-ness. Ping? http://gcc.gnu.org/PR19199 I have further local changes to adjust for other changes that went in, that I'll post upon request, or when the patch goes in.
> Excuse me for asking, but what is it that makes the latest patch I posted not > reasonable for the 4.0 timeframe? The performance regression on C, Java, Ada and fortran code, that isn't affected by this bug. The bug is marked has the "c++" component because it only affects the C++ front-end. A fix that disables MIN_EXPR/MAX_EXPR optimizations in all front-ends is not suitable for a release branch without SPEC testing to show how badly innocent targets will get burnt! There may be lots of places in the Linux kernel that depend upon generating min/max insns for performance reasons... I'd hoped I'd made this clear when I proposed the alternate strategy of only disabling this optimization *only* in the C++ front-end, and *only* prior to tree-ssa. Whilst I agree this is a serious bug, it currently affects all release branches, so taking a universal performance hit to resolve it without considering the consequences seems a bad decision. Just grep for "MIN (" and "MAX (" in GCC's own source code to see how badly this could impact the compiler's own code/performance/bootstrap times.
Subject: Re: [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary roger at eyesopen dot com wrote: > I'd hoped I'd made this clear when I proposed the alternate strategy of only > disabling this optimization *only* in the C++ front-end, and *only* prior to > tree-ssa. Roger, Alexandre, will you to please coordinate to determine who will take responsibility for preparing a patch? What's difficult for me is that I can't quite figure out who's going to actually take this bug and run with it. I think Alexandre is trying hard to fix it, but I'm not confident that he understands what Roger wants. If you two aren't able to work through the issues, or don't have time, please let me know; I want to find some way to get this problem dealt with before 4.0. Thanks,
Subject: Bug 19199 CVSROOT: /cvs/gcc Module name: gcc Changes by: sayle@gcc.gnu.org 2005-04-04 05:02:10 Modified files: gcc : ChangeLog fold-const.c gcc/testsuite : ChangeLog gcc/testsuite/g++.old-deja/g++.oliva: ChangeLog expr2.C Added files: gcc/testsuite/g++.dg/expr: lval2.C Log message: 2005-04-03 Roger Sayle <roger@eyesopen.com> Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * fold-const.c (non_lvalue): Split tests into... (maybe_lvalue_p): New function. (fold_cond_expr_with_comparison): Preserve lvalue-ness for the C++ front-end prior to lowering into gimple form. * g++.dg/expr/lval2.C: New. * expr2.C: Fixed. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.8104&r2=2.8105 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gcc&r1=1.552&r2=1.553 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.5273&r2=1.5274 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/expr/lval2.C.diff?cvsroot=gcc&r1=NONE&r2=1.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog.diff?cvsroot=gcc&r1=1.35&r2=1.36 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C.diff?cvsroot=gcc&r1=1.5&r2=1.6
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold On Apr 4, 2005, Roger Sayle <roger@eyesopen.com> wrote: > (fold_cond_expr_with_comparison): Preserve lvalue-ness for the > C++ front-end prior to lowering into gimple form. > * expr2.C: Fixed. Err... Why did you choose to drop the portion of the patch below, that would have avoided the ugliness of comparing langhooks.name, but still retained it in the ChangeLog entry? Index: fold-const.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v retrieving revision 1.554 diff -u -p -d -u -p -d -u -p -r1.554 fold-const.c --- fold-const.c 4 Apr 2005 08:50:25 -0000 1.554 +++ fold-const.c 4 Apr 2005 13:25:47 -0000 @@ -4173,7 +4173,15 @@ fold_cond_expr_with_comparison (tree typ tree arg00 = TREE_OPERAND (arg0, 0); tree arg01 = TREE_OPERAND (arg0, 1); tree arg1_type = TREE_TYPE (arg1); - tree tem; + tree tem = NULL; + /* If the COND_EXPR can possibly be an lvalue, we don't want to + perform transformations that return a simplified result that will + be recognized as lvalue, but that will not match the expected + result. We may still return other expressions that would be + incorrect, but those are going to be rvalues, and the caller is + supposed to discard them. */ + bool lvalue = !pedantic_lvalues + && maybe_lvalue_p (arg1) && maybe_lvalue_p (arg2); STRIP_NOPS (arg1); STRIP_NOPS (arg2); @@ -4215,10 +4223,12 @@ fold_cond_expr_with_comparison (tree typ case EQ_EXPR: case UNEQ_EXPR: tem = fold_convert (arg1_type, arg1); - return pedantic_non_lvalue (fold_convert (type, negate_expr (tem))); + tem = pedantic_non_lvalue (fold_convert (type, negate_expr (tem))); + break; case NE_EXPR: case LTGT_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg1)); + tem = pedantic_non_lvalue (fold_convert (type, arg1)); + break; case UNGE_EXPR: case UNGT_EXPR: if (flag_trapping_math) @@ -4230,7 +4240,8 @@ fold_cond_expr_with_comparison (tree typ arg1 = fold_convert (lang_hooks.types.signed_type (TREE_TYPE (arg1)), arg1); tem = fold_build1 (ABS_EXPR, TREE_TYPE (arg1), arg1); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); + break; case UNLE_EXPR: case UNLT_EXPR: if (flag_trapping_math) @@ -4241,12 +4252,18 @@ fold_cond_expr_with_comparison (tree typ arg1 = fold_convert (lang_hooks.types.signed_type (TREE_TYPE (arg1)), arg1); tem = fold_build1 (ABS_EXPR, TREE_TYPE (arg1), arg1); - return negate_expr (fold_convert (type, tem)); + tem = negate_expr (fold_convert (type, tem)); + break; default: gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison); break; } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* A != 0 ? A : 0 is simply A, unless A is -0. Likewise A == 0 ? A : 0 is always 0 unless A is -0. Note that both transformations are correct when A is NaN: A != 0 @@ -4255,11 +4272,16 @@ fold_cond_expr_with_comparison (tree typ if (integer_zerop (arg01) && integer_zerop (arg2)) { if (comp_code == NE_EXPR) - return pedantic_non_lvalue (fold_convert (type, arg1)); + tem = pedantic_non_lvalue (fold_convert (type, arg1)); else if (comp_code == EQ_EXPR) - return fold_convert (type, integer_zero_node); + tem = fold_convert (type, integer_zero_node); } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* Try some transformations of A op B ? A : B. A == B? A : B same as B @@ -4309,9 +4331,15 @@ fold_cond_expr_with_comparison (tree typ switch (comp_code) { case EQ_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg2)); + if (lvalue) + break; + tem = pedantic_non_lvalue (fold_convert (type, arg2)); + break; case NE_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg1)); + if (lvalue) + break; + tem = pedantic_non_lvalue (fold_convert (type, arg1)); + break; case LE_EXPR: case LT_EXPR: case UNLE_EXPR: @@ -4327,7 +4355,7 @@ fold_cond_expr_with_comparison (tree typ tem = (comp_code == LE_EXPR || comp_code == UNLE_EXPR) ? fold_build2 (MIN_EXPR, comp_type, comp_op0, comp_op1) : fold_build2 (MIN_EXPR, comp_type, comp_op1, comp_op0); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); } break; case GE_EXPR: @@ -4341,16 +4369,16 @@ fold_cond_expr_with_comparison (tree typ tem = (comp_code == GE_EXPR || comp_code == UNGE_EXPR) ? fold_build2 (MAX_EXPR, comp_type, comp_op0, comp_op1) : fold_build2 (MAX_EXPR, comp_type, comp_op1, comp_op0); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); } break; case UNEQ_EXPR: - if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) - return pedantic_non_lvalue (fold_convert (type, arg2)); + if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) + tem = pedantic_non_lvalue (fold_convert (type, arg2)); break; case LTGT_EXPR: - if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) - return pedantic_non_lvalue (fold_convert (type, arg1)); + if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) + tem = pedantic_non_lvalue (fold_convert (type, arg1)); break; default: gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison); @@ -4358,6 +4386,11 @@ fold_cond_expr_with_comparison (tree typ } } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* If this is A op C1 ? A : C2 with C1 and C2 constant integers, we might still be able to simplify this. For example, if C1 is one less or one more than C2, this might have started @@ -4372,7 +4405,7 @@ fold_cond_expr_with_comparison (tree typ case EQ_EXPR: /* We can replace A with C1 in this case. */ arg1 = fold_convert (type, arg01); - return fold_build3 (COND_EXPR, type, arg0, arg1, arg2); + tem = fold_build3 (COND_EXPR, type, arg0, arg1, arg2); case LT_EXPR: /* If C1 is C2 + 1, this is min(A, C2). */ @@ -4382,8 +4415,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (PLUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold_build2 (MIN_EXPR, - type, arg1, arg2)); + tem = pedantic_non_lvalue (fold_build2 (MIN_EXPR, + type, arg1, arg2)); break; case LE_EXPR: @@ -4394,8 +4427,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (MINUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold_build2 (MIN_EXPR, - type, arg1, arg2)); + tem = pedantic_non_lvalue (fold_build2 (MIN_EXPR, + type, arg1, arg2)); break; case GT_EXPR: @@ -4406,8 +4439,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (MINUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold_build2 (MAX_EXPR, - type, arg1, arg2)); + tem = pedantic_non_lvalue (fold_build2 (MAX_EXPR, + type, arg1, arg2)); break; case GE_EXPR: @@ -4418,8 +4451,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (PLUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold_build2 (MAX_EXPR, - type, arg1, arg2)); + tem = pedantic_non_lvalue (fold_build2 (MAX_EXPR, + type, arg1, arg2)); break; case NE_EXPR: break; @@ -4427,6 +4460,11 @@ fold_cond_expr_with_comparison (tree typ gcc_unreachable (); } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + return NULL_TREE; }
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold On Mon, 4 Apr 2005, Alexandre Oliva wrote: > Err... Why did you choose to drop the portion of the patch below, > that would have avoided the ugliness of comparing langhooks.name, but > still retained it in the ChangeLog entry? pedantic_lvalues means "this is C, until I manage to disentangle enough C/C++/ObjC interactions to keep lvalueness internal to the C front end so pedantic_lvalues can go away". Checking langhooks.name tests for C++, i.e. they differ in their effects for all languages other than C/ObjC/C++: only C/ObjC require that conditional expressions not be turned into lvalues if they weren't such to start with, while only C++ requires that conditional expressions which were lvalues be preserved in their original form. The proper hook to check is one meaning "this is C++" rather than "this is not C", though perhaps adding a new hook "fold_preserve_cond_expr_p" would be cleaner than checking langhooks.name.
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold On Apr 4, 2005, Alexandre Oliva <aoliva@redhat.com> wrote: > On Apr 4, 2005, Roger Sayle <roger@eyesopen.com> wrote: >> long-term solutions have been proposed for g++ 4.1, the patch below is >> the best "4.0 timeframe" work-around that doesn't adversely affect >> performance for GCC's non C++ front-ends, > I don't understand your claim. My patch used pedantic_lvalues, which > is set in such a way by the C and C++ front-ends that makes it even > faster than the string comparison you're using. >> and even retains the ability to synthesize MIN_EXPR and MAX_EXPR for >> C++ during the tree-ssa passes. > This was never removed in my latest patch. BTW, here's a patch for mainline that removes the bogus comment the patch you checked in introduced, and adjusts the lvalueness tests such that they're performed early, and used at any point we need to make a decision on whether the transformation is valid. At the point you perform the test, we may have already stripped some nop_exprs, which might make rvalues look like lvalues, removing some optimization possibilities. Would you like me to add the langhook to this, or do you oppose the entire approach for some reason you still haven't explained? Index: gcc/ChangeLog from Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * fold-const.c (maybe_lvalue_p): Remove bogus comment introduced in previous patch. (fold_cond_expr_with_comparison): Determine whether lvalueness is needed upfront, and proceed accordingly in each case. Index: gcc/fold-const.c =================================================================== RCS file: /cvs/gcc/gcc/gcc/fold-const.c,v retrieving revision 1.554 diff -u -p -r1.554 fold-const.c --- gcc/fold-const.c 4 Apr 2005 08:50:25 -0000 1.554 +++ gcc/fold-const.c 4 Apr 2005 13:42:43 -0000 @@ -2005,7 +2005,6 @@ fold_convert (tree type, tree arg) /* Return false if expr can be assumed not to be an value, true otherwise. */ -/* Return an expr equal to X but certainly not valid as an lvalue. */ static bool maybe_lvalue_p (tree x) @@ -4173,7 +4172,15 @@ fold_cond_expr_with_comparison (tree typ tree arg00 = TREE_OPERAND (arg0, 0); tree arg01 = TREE_OPERAND (arg0, 1); tree arg1_type = TREE_TYPE (arg1); - tree tem; + tree tem = NULL; + /* If the COND_EXPR can possibly be an lvalue, we don't want to + perform transformations that return a simplified result that will + be recognized as lvalue, but that will not match the expected + result. We may still return other expressions that would be + incorrect, but those are going to be rvalues, and the caller is + supposed to discard them. */ + bool lvalue = !pedantic_lvalues + && maybe_lvalue_p (arg1) && maybe_lvalue_p (arg2); STRIP_NOPS (arg1); STRIP_NOPS (arg2); @@ -4215,10 +4222,12 @@ fold_cond_expr_with_comparison (tree typ case EQ_EXPR: case UNEQ_EXPR: tem = fold_convert (arg1_type, arg1); - return pedantic_non_lvalue (fold_convert (type, negate_expr (tem))); + tem = pedantic_non_lvalue (fold_convert (type, negate_expr (tem))); + break; case NE_EXPR: case LTGT_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg1)); + tem = pedantic_non_lvalue (fold_convert (type, arg1)); + break; case UNGE_EXPR: case UNGT_EXPR: if (flag_trapping_math) @@ -4230,7 +4239,8 @@ fold_cond_expr_with_comparison (tree typ arg1 = fold_convert (lang_hooks.types.signed_type (TREE_TYPE (arg1)), arg1); tem = fold_build1 (ABS_EXPR, TREE_TYPE (arg1), arg1); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); + break; case UNLE_EXPR: case UNLT_EXPR: if (flag_trapping_math) @@ -4241,12 +4251,18 @@ fold_cond_expr_with_comparison (tree typ arg1 = fold_convert (lang_hooks.types.signed_type (TREE_TYPE (arg1)), arg1); tem = fold_build1 (ABS_EXPR, TREE_TYPE (arg1), arg1); - return negate_expr (fold_convert (type, tem)); + tem = negate_expr (fold_convert (type, tem)); + break; default: gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison); break; } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* A != 0 ? A : 0 is simply A, unless A is -0. Likewise A == 0 ? A : 0 is always 0 unless A is -0. Note that both transformations are correct when A is NaN: A != 0 @@ -4255,11 +4271,16 @@ fold_cond_expr_with_comparison (tree typ if (integer_zerop (arg01) && integer_zerop (arg2)) { if (comp_code == NE_EXPR) - return pedantic_non_lvalue (fold_convert (type, arg1)); + tem = pedantic_non_lvalue (fold_convert (type, arg1)); else if (comp_code == EQ_EXPR) - return fold_convert (type, integer_zero_node); + tem = fold_convert (type, integer_zero_node); } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* Try some transformations of A op B ? A : B. A == B? A : B same as B @@ -4286,13 +4307,7 @@ fold_cond_expr_with_comparison (tree typ a number and A is not. The conditions in the original expressions will be false, so all four give B. The min() and max() versions would give a NaN instead. */ - if (operand_equal_for_comparison_p (arg01, arg2, arg00) - /* Avoid these transformations if the COND_EXPR may be used - as an lvalue in the C++ front-end. PR c++/19199. */ - && (in_gimple_form - || strcmp (lang_hooks.name, "GNU C++") != 0 - || ! maybe_lvalue_p (arg1) - || ! maybe_lvalue_p (arg2))) + if (operand_equal_for_comparison_p (arg01, arg2, arg00)) { tree comp_op0 = arg00; tree comp_op1 = arg01; @@ -4309,9 +4324,15 @@ fold_cond_expr_with_comparison (tree typ switch (comp_code) { case EQ_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg2)); + if (lvalue) + break; + tem = pedantic_non_lvalue (fold_convert (type, arg2)); + break; case NE_EXPR: - return pedantic_non_lvalue (fold_convert (type, arg1)); + if (lvalue) + break; + tem = pedantic_non_lvalue (fold_convert (type, arg1)); + break; case LE_EXPR: case LT_EXPR: case UNLE_EXPR: @@ -4327,7 +4348,7 @@ fold_cond_expr_with_comparison (tree typ tem = (comp_code == LE_EXPR || comp_code == UNLE_EXPR) ? fold_build2 (MIN_EXPR, comp_type, comp_op0, comp_op1) : fold_build2 (MIN_EXPR, comp_type, comp_op1, comp_op0); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); } break; case GE_EXPR: @@ -4341,16 +4362,16 @@ fold_cond_expr_with_comparison (tree typ tem = (comp_code == GE_EXPR || comp_code == UNGE_EXPR) ? fold_build2 (MAX_EXPR, comp_type, comp_op0, comp_op1) : fold_build2 (MAX_EXPR, comp_type, comp_op1, comp_op0); - return pedantic_non_lvalue (fold_convert (type, tem)); + tem = pedantic_non_lvalue (fold_convert (type, tem)); } break; case UNEQ_EXPR: - if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) - return pedantic_non_lvalue (fold_convert (type, arg2)); + if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) + tem = pedantic_non_lvalue (fold_convert (type, arg2)); break; case LTGT_EXPR: - if (!HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) - return pedantic_non_lvalue (fold_convert (type, arg1)); + if (! lvalue && !HONOR_NANS (TYPE_MODE (TREE_TYPE (arg1)))) + tem = pedantic_non_lvalue (fold_convert (type, arg1)); break; default: gcc_assert (TREE_CODE_CLASS (comp_code) == tcc_comparison); @@ -4358,6 +4379,11 @@ fold_cond_expr_with_comparison (tree typ } } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + /* If this is A op C1 ? A : C2 with C1 and C2 constant integers, we might still be able to simplify this. For example, if C1 is one less or one more than C2, this might have started @@ -4372,7 +4398,7 @@ fold_cond_expr_with_comparison (tree typ case EQ_EXPR: /* We can replace A with C1 in this case. */ arg1 = fold_convert (type, arg01); - return fold_build3 (COND_EXPR, type, arg0, arg1, arg2); + tem = fold_build3 (COND_EXPR, type, arg0, arg1, arg2); case LT_EXPR: /* If C1 is C2 + 1, this is min(A, C2). */ @@ -4382,8 +4408,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (PLUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold_build2 (MIN_EXPR, - type, arg1, arg2)); + tem = pedantic_non_lvalue (fold_build2 (MIN_EXPR, + type, arg1, arg2)); break; case LE_EXPR: @@ -4394,8 +4420,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (MINUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold_build2 (MIN_EXPR, - type, arg1, arg2)); + tem = pedantic_non_lvalue (fold_build2 (MIN_EXPR, + type, arg1, arg2)); break; case GT_EXPR: @@ -4406,8 +4432,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (MINUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold_build2 (MAX_EXPR, - type, arg1, arg2)); + tem = pedantic_non_lvalue (fold_build2 (MAX_EXPR, + type, arg1, arg2)); break; case GE_EXPR: @@ -4418,8 +4444,8 @@ fold_cond_expr_with_comparison (tree typ const_binop (PLUS_EXPR, arg2, integer_one_node, 0), OEP_ONLY_CONST)) - return pedantic_non_lvalue (fold_build2 (MAX_EXPR, - type, arg1, arg2)); + tem = pedantic_non_lvalue (fold_build2 (MAX_EXPR, + type, arg1, arg2)); break; case NE_EXPR: break; @@ -4427,6 +4453,11 @@ fold_cond_expr_with_comparison (tree typ gcc_unreachable (); } + if (tem && (! lvalue || maybe_lvalue_p (tem))) + return tem; + else + tem = NULL; + return NULL_TREE; } -- Alexandre Oliva http://www.ic.unicamp.br/~oliva/ Red Hat Compiler Engineer aoliva@{redhat.com, gcc.gnu.org} Free Software Evangelist oliva@{lsd.ic.unicamp.br, gnu.org}
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold On Apr 4, 2005, Alexandre Oliva <aoliva@redhat.com> wrote: > + result. We may still return other expressions that would be > + incorrect, but those are going to be rvalues, and the caller is > + supposed to discard them. */ Uhh... This sentence is no longer true. I'm removing it from my local copy of the patch.
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold Hi Alex, My apologies yet again for not being more explicit about all of the things that were wrong (and or I was unhappy with) with your proposed solution. I'd hoped that I was clear in the comments in the bugzilla thread, and that you'd appreciate the issues it addressed. Problems with your approach: [1] The use of pedantic_lvalues to identify the non-C front-ends, adversely affects code generation in the Java, Fortran and Ada front-ends. The use of COND_EXPRs as lvalues is unique to the C++ front-end, so ideally a fix shouldn't regress code quality on innocent front-ends. Certainly, not without benchmarking to indicate how significant a performance hit, these other languages are taking prior to a release. [2] The pedantic_lvalues flag is itself a hack used by the C front-end, that is currently being removed by Joseph in his clean-up of the C parser. Adding this use would block his efforts, until an alternate solution is found. Admittedly, this isn't an issue for the 4.0 release, but creates more work or a regression once this is removed from mainline. [3] Your patch is too invasive. Compared to the four-line counter proposal that disables just the problematic class of transformations, your much larger patch inherently contains a much larger risk. For example, there is absolutely no need to modify the source code on the "A >= 0 ? A : -A -> abs (A)" paths as these transformations could never interfere with the lvalueness of an expression. Additionally, once one of the longer term solutions proposed by Mark or me is implemented, all of these workarounds will have to be undone/ reverted. By only affecting a single clause, we avoid the potential for leaving historic code bit-rotting in the tree. [4] Despite your counter claims you approach *does* inhibit the ability of the the tree-ssa optimizers to synthesize MIN_EXPR and MAX_EXPR nodes. Once the in_gimple_form flag is set, fold-const.c is able to optimize "A == B ? A : B -> B" even when compiling C++, as it knows that a COND_EXPR can't be used as an lvalue in the late middle-end. [5] For the immediate term, I don't think its worth worrying about converting non-lvalues into lvalues, the wrong-code bugs and diagnostic issues are related solely to the lvalue -> non-lvalue transition. At this stage of pre-release, lower risk changes are cleary preferrable, and making this change will break code that may have erroneously compiled in the past. Probably, OK for 4.0, but not for 3.4 (which also exhibits this problem). And although, not serious enough to warrant a [6], it should be pointed out that several of your recent patches have introduced regressions. Indeed, you've not yet reported that your patch has been sucessfully bootstraped or regression tested on any target triple. Indeed, your approach of posting a patch before completing the prerequisite testing sprecified/stressed in contribute.html, on more than one occassion recently resulted in the patch having to be rewritten/tweaked. Indeed, as witnessed in "comment #17", I've already approved an earlier version your patch once, only to later discover you were wasting my time. As a middle-end maintainer, having been pinged by the release manager that we/you weren't making sufficient progress towards addressing the issues with your patch, I took the opportunity to apply a fix that was within my authority to commit. If you'd had made and tested the changes that I requested in a timely manner, I'm sure I'd have approved those efforts by now. My apologies again for not being blunt earlier. My intention was that by listing all of the benefits of an alternate approach in comment #19 of the bugzilla PR, I wouldn't have to explicitly list them as the defficiencies of your approach. Some people prefer the carrot to the stick with patch reviews [others like RTH's "No"]. Perhaps, I should ask the counter question to your comment #21? In what way do you feel that the committed patch isn't clearly superior to your proposed solution? p.s. Thanks for spotting my mistake of leaving a bogus comment above maybe_lvalue_p. Roger --
Subject: Re: [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary Roger -- Thank you for fixing this PR! Very much appreciated. If I'm reading correctly, the patch is now on the mainline, so the 4.1 regression listing in the subject line is now incorrect? Your patch is definitely suitable for 4.0 as well; I shall look forward to seeing it there. Thanks,
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold On Apr 4, 2005, Roger Sayle <roger@eyesopen.com> wrote: > My apologies yet again for not being more explicit about all of the > things that were wrong (and or I was unhappy with) with your proposed > solution. I'd hoped that I was clear in the comments in the bugzilla > thread, and that you'd appreciate the issues it addressed. I didn't. In fact, I only saw your responses in bugzilla early last week, since bugzilla e-mail I get goes to a folder that I don't read very often because of all the traffic in gcc-prs. > [1] The use of pedantic_lvalues to identify the non-C front-ends, > adversely affects code generation in the Java, Fortran and Ada > front-ends. Indeed. I'd misunderstood what pedantic_lvalues stood for, and thought it could be used to distinguish C++ from other languages, not C from other languages. My mistake. That's why it was never clear to me that you disliked its use, and why. > The use of COND_EXPRs as lvalues is unique to the > C++ front-end I don't know about that. Among all the languages supported by GCC, I'm only familiar with C, C++ and Java. > [3] Your patch is too invasive. Compared to the four-line counter > proposal that disables just the problematic class of transformations, > your much larger patch inherently contains a much larger risk. For > example, there is absolutely no need to modify the source code on the > "A >= 0 ? A : -A -> abs (A)" paths as these transformations could > never interfere with the lvalueness of an expression. Granted. OTOH, the patch faulted in trying to be overly consistent in potentially-broken code, as opposed to keeping potentially-broken code broken. > Additionally, once one of the longer term solutions proposed by Mark > or me is implemented, all of these workarounds will have to be undone/ > reverted. By only affecting a single clause, we avoid the potential > for leaving historic code bit-rotting in the tree. If you're convinced that's the only clause that triggers the problem, great. I wasn't. > [4] Despite your counter claims you approach *does* inhibit the ability > of the the tree-ssa optimizers to synthesize MIN_EXPR and MAX_EXPR > nodes. Indeed. I still had the behavior from an earlier version of the patch, in which cond_expr returned different values from maybe_lvalue_p depending on whether we were in gimple form or not. Sorry about that. > [5] For the immediate term, I don't think its worth worrying about > converting non-lvalues into lvalues, No worries here, all of my effort was toward preserving lvalueness. > And although, not serious enough to warrant a [6], it should be pointed > out that several of your recent patches have introduced regressions. Oh, like the patch in which I did check in a testcase after bootstrapping it on amd64-linux-gnu, although I didn't realize it still failed on i686-pc-linux-gnu, a host on which I don't bootstrap very often any more because my 32-bit hardware has become too slow? > Indeed, you've not yet reported that your patch has been sucessfully > bootstraped or regression tested on any target triple. Did I have to, before checking the patch in? I didn't think so. > Indeed, your > approach of posting a patch before completing the prerequisite testing > sprecified/stressed in contribute.html, on more than one occassion > recently resulted in the patch having to be rewritten/tweaked. And the need for rewriting/tweaking is exactly the reason why I choose to post patches early. Quite often, patches don't go in as they're posted, since maintainers often request additional minor tweaks. At about 24 hours per test cycle, and about as much for patch review, I don't think posting a patch proposal early would hurt anyone. In general, I'm asking for feedback on the approach, rather than on the exact patch spelling. If people find the patch to be good as is, great. Otherwise, sometimes I can still tweak the patch and get it into the next build&test cycle. And, more importantly, I like to describe the problem and the solution while it's still fresh in my mind. If I wait 1 or 2 days to post it, I won't be as precise in my description, and if questions arise, I may fail to answer correctly. > Indeed, as witnessed in "comment #17", I've already approved an > earlier version your patch once, only to later discover you were > wasting my time. If you find that reviewing patch proposals is wasting your time, I'm sorry. In general, I run a `make all; make check-gcc or make check-g++' on a previously-built tree before starting a full bootstrap and posting the patch, but this is not enough to catch all errors. In fact, a full bootstrap and test cycle isn't enough either, since we don't all use the same hosts. So, patches are going to be posted that need revision. Since most of my patches have minor revisions requested, not posting the patch for early feedback wastes a lot of my time. How do we find a balance? > As a middle-end maintainer, having been pinged by the release manager > that we/you weren't making sufficient progress towards addressing the > issues with your patch, Only days after that did I find you'd added notes to bugzilla, while an e-mail reply would have caught my attention immediately. > Perhaps, I should ask the counter question to your comment #21? > In what way do you feel that the committed patch isn't clearly > superior to your proposed solution? p.s. Thanks for spotting my > mistake of leaving a bogus comment above maybe_lvalue_p. I can't immediately convince myself that it covers all the potentially-faulty cases, and it tests for lvalueness after stripping NOP_EXPRs, which may prevent optimization in case the operand of the NOP_EXPR looks like an lvalue, but the NOP_EXPR itself is clearly an rvalue. It's easy enough to get the lvalue variable to take in_gimple_form into account, and we can move the test for the language name to the assignment to lvalue instead of using pedantic_lvalues, if you really think that's better than a langhook. Personally, I'd go with a lang-specific boolean that defaults to false, to tell whether a cond_expr/min_expr/max_expr can be an lvalue. I thought pedantic_lvalues was just that, but it's now obvious that I was mistaken.
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold Alexandre Oliva wrote: > If you find that reviewing patch proposals is wasting your time, I'm > sorry. Of course, untested patches should be clearly marked as such, which yours always are. Given that, I don't feel like you're wasting my time with such a posting; if I see something obviously disconcerting I can comment. However, I do find that I'm less interested in reviewing such patches. So, from my point of view, I'd prefer that you post most patches after a full test cycle.
Subject: Bug 19199 CVSROOT: /cvs/gcc Module name: gcc Branch: gcc-4_0-branch Changes by: sayle@gcc.gnu.org 2005-04-05 05:15:23 Modified files: gcc : ChangeLog fold-const.c gcc/testsuite : ChangeLog gcc/testsuite/g++.old-deja/g++.oliva: ChangeLog expr2.C Added files: gcc/testsuite/g++.dg/expr: lval2.C Log message: 2005-04-04 Roger Sayle <roger@eyesopen.com> Alexandre Oliva <aoliva@redhat.com> PR c++/19199 * fold-const.c (non_lvalue): Split tests into... (maybe_lvalue_p): New function. (fold_cond_expr_with_comparison): Preserve lvalue-ness for the C++ front-end prior to lowering into gimple form. * g++.dg/expr/lval2.C: New. * expr2.C: Fixed. Patches: http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.123&r2=2.7592.2.124 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.517.2.5&r2=1.517.2.6 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5084.2.96&r2=1.5084.2.97 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/expr/lval2.C.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=NONE&r2=1.1.2.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.35&r2=1.35.62.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.5&r2=1.5.76.1
Now that a fix has been applied to both mainline and the 4.0 branch, I've been investigating backporting the fix to 3.4. Unfortunately, a significant feature of the fixes proposed by Alex and me are that they disable the problematic transformations during parsing, but allow them to be caught by later tree-ssa passes. Unfortunately, in gcc 3.4.x if we disable constant folding of COND_EXPRs for C++, there are no tree-level optimizers that can take up the slack, and instead we'd rely on if-conversion to do what it can at the RTL level. I'm not convinced that 3.4 if-conversion is particularly effective in this respect (certainly across all targets), and I've not yet checked whether all of the affected transformations are implementated in ifcvt.c (even on mainline). Might I propose that we close this bug as won't fix for both the 3.3 and 3.4 branches. GCC has always generated wrong code for this case, and the current state of 3.4.4 is that we issue an incorrect warning (which seems better than silently generating wrong code as we did with 3.1). It's a trade-off; but I'm keen to avoid degrading g++ 3.4.5 (in the common case), for a diagnostic regression. Or we could leave the PR open (and perhaps unassign it) in the hope that a 3.4 solution will be discovered, as the mainline/4.x fixes aren't suitable. Mark? Alexandre?
Subject: Re: [3.3/3.4 Regression] Wrong warning about returning a reference to a temporary roger at eyesopen dot com wrote: > Might I propose that we close this bug as won't fix for both the 3.3 and 3.4 > branches. GCC has always generated wrong code for this case, and the current > state of 3.4.4 is that we issue an incorrect warning (which seems better than > silently generating wrong code as we did with 3.1). Just so that I'm clear, what I think you're saying that the wrong-code problem has been there all along, and the only regression was actually the warning. Investigating the warning revealed that we were generating wrong-code, and fixing the wrong-code problem fixed the warning, but the only actual regression per se was the warning. If that's accurate, then, yes, we should add a note saying that this won't be fixed for 3.4.x, and close the PR as FIXED. Thanks,
That's my interpretation of (Andrew Pinki's) comment #6 in the bugzilla PR [n.b. I haven't reconfirmed his analysis personally]
As per comment #37, closing as WONTFIX.