[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From aoliva at gcc dot gnu dot org 2005-04-04 13:32 --- Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold On Apr 4, 2005, Roger Sayle [EMAIL PROTECTED] 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.c4 Apr 2005 08:50:25 - 1.554 +++ fold-const.c4 Apr 2005 13:25:47 - @@ -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 : Bsame 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 =
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From joseph at codesourcery dot com 2005-04-04 13:41 --- 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From aoliva at gcc dot gnu dot org 2005-04-04 13:50 --- Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold On Apr 4, 2005, Alexandre Oliva [EMAIL PROTECTED] wrote: On Apr 4, 2005, Roger Sayle [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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 - 1.554 +++ gcc/fold-const.c 4 Apr 2005 13:42:43 - @@ -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 =
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From aoliva at gcc dot gnu dot org 2005-04-04 15:02 --- Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold On Apr 4, 2005, Alexandre Oliva [EMAIL PROTECTED] 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From roger at eyesopen dot com 2005-04-04 16:02 --- 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 -- -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From mark at codesourcery dot com 2005-04-04 16:39 --- 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, -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From mark at codesourcery dot com 2005-04-04 00:37 --- 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, -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From cvs-commit at gcc dot gnu dot org 2005-04-04 05:02 --- Subject: Bug 19199 CVSROOT:/cvs/gcc Module name:gcc Changes by: [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Alexandre Oliva [EMAIL PROTECTED] 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=gccr1=2.8104r2=2.8105 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/fold-const.c.diff?cvsroot=gccr1=1.552r2=1.553 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gccr1=1.5273r2=1.5274 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/expr/lval2.C.diff?cvsroot=gccr1=NONEr2=1.1 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/ChangeLog.diff?cvsroot=gccr1=1.35r2=1.36 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.old-deja/g++.oliva/expr2.C.diff?cvsroot=gccr1=1.5r2=1.6 -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From aoliva at gcc dot gnu dot org 2005-04-02 17:29 --- 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 [EMAIL PROTECTED] 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From roger at eyesopen dot com 2005-04-03 03:20 --- 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- 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? -- What|Removed |Added AssignedTo|unassigned at gcc dot gnu |aoliva at gcc dot gnu dot |dot org |org Status|NEW |ASSIGNED Last reconfirmed|2004-12-30 14:48:58 |2005-03-30 02:55:13 date|| http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From mark at codesourcery dot com 2005-03-30 07:20 --- 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From roger at eyesopen dot com 2005-03-25 06:03 --- 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From mark at codesourcery dot com 2005-03-25 06:29 --- 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, -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From aoliva at gcc dot gnu dot org 2005-03-08 23:23 --- 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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] * 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 - 1.535 +++ gcc/fold-const.c 8 Mar 2005 22:07:52 - @@ -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 [EMAIL PROTECTED] PR c++/19199 *
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From roger at eyesopen dot com 2005-03-09 01:28 --- 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 -- -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From aoliva at gcc dot gnu dot org 2005-03-09 04:11 --- 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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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 - 1.535 +++ gcc/fold-const.c 9 Mar 2005 03:59:18 - @@ -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,
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From aoliva at gcc dot gnu dot org 2005-03-07 03:28 --- 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 [EMAIL PROTECTED] 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From mark at codesourcery dot com 2005-03-07 04:19 --- 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 [EMAIL PROTECTED] 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From mark at codesourcery dot com 2005-03-06 00:14 --- 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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From mmitchel at gcc dot gnu dot org 2005-03-04 08:13 --- (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. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
[Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary
--- Additional Comments From aoliva at gcc dot gnu dot org 2005-03-04 19:08 --- 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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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 - 1.523 +++ gcc/fold-const.c 4 Mar 2005 19:04:26 - @@ -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)))