gcc trunk, x86_64. The test case is simple and my understanding that the correct result is 0x01000000, while gcc produces 0. Slight massaging of the code (like removal of "* m2") changes the behaviour to be correct. Not sure whom to blame - front-end or optimizations (even though it's -O0). > cat f.cpp #include <stdio.h> unsigned long long int m2 = 1; int xxx = 0x01000000; int main() { int a = ( ( (char) xxx) ? 0 : xxx) * m2; printf("0x%x (expected 0x01000000)\n", a); return 0; } > g++ -O0 f.cpp -o out; ./out; 0x0 (expected 0x01000000) > clang++ -O0 f.cpp -o out; ./out 0x1000000 (expected 0x01000000)
That really looks like a bug. Even gcc34 prints 0!
Probably started with r125012.
Ugh, I believe fold_cond_expr_with_comparison does Very Bad stuff: it sees A == 0 ? A : 0 and thinks that it can be optimized to 0, btu it can't, in this case we have (signed char) xxx == 0 ? (unsigned long long) xxx : 0 (signed char) xxx is indeed 0 but (unsigned long long) xxx is not.
Per n4659 7.8/[conv.integral]: ``` If the destination type is signed, the value is unchanged if it can be represented in the destination type; otherwise, the value is implementation-defined. ``` Isn't `(char)xxx` implementation-defined? (as GCC treats `char` as signed) And as the following code prints 0, I believe that GCC defines the result of such casting as 0: ``` #include <stdio.h> int xxx = 0x01000000; int main() { printf("%d\n", (char)xxx); return 0; } ``` So I think the output should indeed be 0.
Oops, sorry, I read the 2nd and the 3rd operand of the conditional operator in wrong order. A silly mistake..
The folding obviously preserves precision changing casts, so it should be valid. Things must go wrong elsewhere, possibly in operand_equal_for_comparison_p which ends up using get_narrower.
Yes, it's in operand_equal_for_comparison_p, and that would also explain why it started with r125012. I didn't update the BZ when I found this out. I hope to look into this more this week.
I wonder if I could just --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -3401,14 +3401,14 @@ operand_equal_for_comparison_p (tree arg0, tree arg1, tree other) primarg1 = get_narrower (arg1, &unsignedp1); primother = get_narrower (other, &unsignedpo); + tree type = TREE_TYPE (arg0); correct_width = TYPE_PRECISION (TREE_TYPE (arg1)); if (unsignedp1 == unsignedpo + && TYPE_PRECISION (TREE_TYPE (primarg1)) == TYPE_PRECISION (type) && TYPE_PRECISION (TREE_TYPE (primarg1)) < correct_width && TYPE_PRECISION (TREE_TYPE (primother)) < correct_width) { - tree type = TREE_TYPE (arg0); - /* Make sure shorter operand is extended the right way to match the longer operand. */ primarg1 = fold_convert (signed_or_unsigned_type_for so far it seems to work.
(In reply to Marek Polacek from comment #8) > I wonder if I could just > > --- a/gcc/fold-const.c > +++ b/gcc/fold-const.c > @@ -3401,14 +3401,14 @@ operand_equal_for_comparison_p (tree arg0, tree > arg1, tree other) > > primarg1 = get_narrower (arg1, &unsignedp1); > primother = get_narrower (other, &unsignedpo); > + tree type = TREE_TYPE (arg0); > > correct_width = TYPE_PRECISION (TREE_TYPE (arg1)); > if (unsignedp1 == unsignedpo > + && TYPE_PRECISION (TREE_TYPE (primarg1)) == TYPE_PRECISION (type) > && TYPE_PRECISION (TREE_TYPE (primarg1)) < correct_width > && TYPE_PRECISION (TREE_TYPE (primother)) < correct_width) > { > - tree type = TREE_TYPE (arg0); > - > /* Make sure shorter operand is extended the right way > to match the longer operand. */ > primarg1 = fold_convert (signed_or_unsigned_type_for > > so far it seems to work. Looks a bit too conservative to me, without explanation what exactly goes wrong here. The code might be confused where it does the extension. Are there any testcases that break when we change operand_equal_for_comparison to just do operand_equal_p?
Nope, nothing breaks. It occurred to me that we might get rid of it when I found out that the code comes from rms, 1992. :)
On Thu, 17 Aug 2017, mpolacek at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81814 > > Marek Polacek <mpolacek at gcc dot gnu.org> changed: > > What |Removed |Added > ---------------------------------------------------------------------------- > Status|NEW |ASSIGNED > Assignee|unassigned at gcc dot gnu.org |mpolacek at gcc dot gnu.org > > --- Comment #10 from Marek Polacek <mpolacek at gcc dot gnu.org> --- > Nope, nothing breaks. It occurred to me that we might get rid of it when I > found out that the code comes from rms, 1992. :) I'm all for it - maybe you can do one further verification step in instrumenting the number of cases it triggers during bootstrap? Might be a bit awkward to only detect the cases where an actual transform happens ...
Sure, let me see.
Author: mpolacek Date: Thu Aug 17 14:33:13 2017 New Revision: 251152 URL: https://gcc.gnu.org/viewcvs?rev=251152&root=gcc&view=rev Log: PR middle-end/81814 * fold-const.c (operand_equal_for_comparison_p): Remove code that used to mimic what shorten_compare did. Change the return type to bool. (fold_cond_expr_with_comparison): Update call to operand_equal_for_comparison_p. (fold_ternary_loc): Likewise. * gcc.dg/torture/pr81814.c: New test. Added: trunk/gcc/testsuite/gcc.dg/torture/pr81814.c Modified: trunk/gcc/ChangeLog trunk/gcc/fold-const.c trunk/gcc/testsuite/ChangeLog
Fixed for GCC 8.
Author: aldyh Date: Wed Sep 13 17:06:20 2017 New Revision: 252467 URL: https://gcc.gnu.org/viewcvs?rev=252467&root=gcc&view=rev Log: PR middle-end/81814 * fold-const.c (operand_equal_for_comparison_p): Remove code that used to mimic what shorten_compare did. Change the return type to bool. (fold_cond_expr_with_comparison): Update call to operand_equal_for_comparison_p. (fold_ternary_loc): Likewise. * gcc.dg/torture/pr81814.c: New test. Added: branches/range-gen2/gcc/testsuite/gcc.dg/torture/pr81814.c Modified: branches/range-gen2/gcc/ChangeLog branches/range-gen2/gcc/fold-const.c branches/range-gen2/gcc/testsuite/ChangeLog