Seen on: 18.04.3 LTS (Bionic Beaver) kar@kar-VirtualBox:~/example1$ gcc-9 ex1.c -o ex kar@kar-VirtualBox:~/example1$ ./ex 0 kar@kar-VirtualBox:~/example1$ gcc-7 ex1.c -o ex kar@kar-VirtualBox:~/example1$ ./ex 2 kar@kar-VirtualBox:~/example1$ more ex1.c #include <stdio.h> typedef int int32_t; int main() { int a = 0; int32_t b = 0; (a > 0) * (b |= 2); printf("%d\n", b); } === 1. Output shall be 2 instead of 0. 2. GCC-8 and GCC-9 produce 0. gcc-9 (Ubuntu 9.2.1-17ubuntu1~18.04.1) 9.2.1 20191102 gcc-8 (Ubuntu 8.3.0-6ubuntu1~18.04.1) 8.3.0 gcc-7 (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0 compiled: gcc-9 ex1.c -o ex 3. Explanation: (a > 0) * (b |= 2); ==> b here shall be set to 2, but since (a > 0) is 0, the side effect of evaluating b is skipped. As a result printf("%d\n", b); prints to the screen 0 instead of 2. 4. GCC-7 and LLVM (for example) produce 2. Note: You can try to compile the code with GCC-7 or llvm to observe the behaviour where the output is 2.
Started with r8-2090-g2071f8f980cc0de02af3d7d7de201f4f189058ff typedef int I; int main () { int a = 0; I b = 0; (a > 0) * (b |= 2); if (b != 2) __builtin_abort (); return 0; }
(In reply to Jakub Jelinek from comment #1) > Started with r8-2090-g2071f8f980cc0de02af3d7d7de201f4f189058ff I was thinking it was that and I was looking for that part in match.pd earlier. There are two ways of fixing this I can see; checking for side effects or just having this pattern only for GIMPLE.
Well, genmatch has some code to add TREE_SIDE_EFFECTS checks on its own, but apparently it doesn't trigger in this case, perhaps it needs be conditional in the matching pattern rather than replacement or it needs to be matched more than once. --- gcc/match.pd.jj 2020-02-05 11:12:33.679383217 +0100 +++ gcc/match.pd 2020-02-14 22:31:46.416421704 +0100 @@ -1472,7 +1472,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (for cmp (gt lt ge le) (simplify (mult (convert (cmp @0 @1)) @2) - (cond (cmp @0 @1) @2 { build_zero_cst (type); }))) + (if (GIMPLE || !TREE_SIDE_EFFECTS (@2)) + (cond (cmp @0 @1) @2 { build_zero_cst (type); })))) /* For integral types with undefined overflow and C != 0 fold x * C EQ/NE y * C into x EQ/NE y. */ of course fixes it, will bootstrap/regtest it with the testcase and let's see what Richi will say ;).
Note that the pattern will get sign of floating-point zero wrong if it ever triggers for an fp type. Also, it's too specific: it at least misses eq/ne comparisons, but generally speaking it doesn't need to be tied to comparisons in the first place: any multiplication by a boolean value can be converted to a select.
Unfortunately, code inspection finds other problematic patterns, e.g. since my r10-5256-g49647b7b25673273262fb630598027c6d841690f change: int w; int foo (int x, int y, int z) { int r = z - ((z - w++) & -(x < y)); return r; } int main () { w = 4; if (foo (5, 7, 12) != 4 || w != 5) __builtin_abort (); if (foo (7, 5, 12) != 12 || w != 6) __builtin_abort (); return 0; } or int w; int foo (int x, int y, int z) { int r = z + ((w++ - z) & -(x < y)); return r; } int main () { w = 4; if (foo (5, 7, 12) != 4 || w != 5) __builtin_abort (); if (foo (7, 5, 12) != 12 || w != 6) __builtin_abort (); return 0; }
Created attachment 47844 [details] gcc10-pr93744.patch Untested fix. I'd say issues with that pattern unrelated to side-effects should be tracked independently.
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>: https://gcc.gnu.org/g:187dd955dbee3939c3a2ca7b6839e7f709999125 commit r10-6646-g187dd955dbee3939c3a2ca7b6839e7f709999125 Author: Jakub Jelinek <jakub@redhat.com> Date: Sat Feb 15 12:53:44 2020 +0100 match.pd: Disallow side-effects in GENERIC for non-COND_EXPR to COND_EXPR simplifications [PR93744] As the following testcases show (the first one reported, last two found by code inspection), we need to disallow side-effects in simplifications that turn some unconditional expression into conditional one. From my little understanding of genmatch.c, it is able to automatically disallow side effects if the same operand is used multiple times in the match pattern, maybe if it is used multiple times in the replacement pattern, and if it is used in conditional contexts in the match pattern, could it be taught to handle this case too? If yes, perhaps just the first hunk could be usable for 8/9 backports (+ the testcases). 2020-02-15 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/93744 * match.pd (((m1 >/</>=/<= m2) * d -> (m1 >/</>=/<= m2) ? d : 0, A - ((A - B) & -(C cmp D)) -> (C cmp D) ? B : A, A + ((B - A) & -(C cmp D)) -> (C cmp D) ? B : A): For GENERIC, make sure @2 in the first and @1 in the other patterns has no side-effects. * gcc.c-torture/execute/pr93744-1.c: New test. * gcc.c-torture/execute/pr93744-2.c: New test. * gcc.c-torture/execute/pr93744-3.c: New test.
As for the signed zeros, I can't really reproduce it. Tried with: __attribute__((noipa)) void foo (int x) { double a = 0.0; double b = -0.0; _Bool c = x > 0; _Bool d = x > 1; double e = c; double f = d; double g = e * a; double h = f * b; if (__builtin_copysign (1.0, g) != 1.0 || __builtin_copysign (1.0, h) != -1.0) __builtin_abort (); } int main () { foo (0); return 0; } The thing is that convert in (convert (cmp $0 $1)) seems to match just CASE_CONVERT:, but result of a comparison is always integral (or vector integral) and so there would need to be a FLOAT_EXPR rather than {NOP,CONVERT}_EXPR to convert it to floating point. Allowing other comparisons or any boolean results is certainly reasonable, but would be an enhancement request for GCC 11.
The releases/gcc-9 branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>: https://gcc.gnu.org/g:498055331393d3c32fea5a6142c926b6a7700b8d commit r9-8241-g498055331393d3c32fea5a6142c926b6a7700b8d Author: Jakub Jelinek <jakub@redhat.com> Date: Sat Feb 15 12:53:44 2020 +0100 match.pd: Disallow side-effects in GENERIC for non-COND_EXPR to COND_EXPR simplifications [PR93744] As the following testcases show (the first one reported, last two found by code inspection), we need to disallow side-effects in simplifications that turn some unconditional expression into conditional one. From my little understanding of genmatch.c, it is able to automatically disallow side effects if the same operand is used multiple times in the match pattern, maybe if it is used multiple times in the replacement pattern, and if it is used in conditional contexts in the match pattern, could it be taught to handle this case too? If yes, perhaps just the first hunk could be usable for 8/9 backports (+ the testcases). 2020-02-15 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/93744 * match.pd (((m1 >/</>=/<= m2) * d -> (m1 >/</>=/<= m2) ? d : 0, A - ((A - B) & -(C cmp D)) -> (C cmp D) ? B : A, A + ((B - A) & -(C cmp D)) -> (C cmp D) ? B : A): For GENERIC, make sure @2 in the first and @1 in the other patterns has no side-effects. * gcc.c-torture/execute/pr93744-1.c: New test. * gcc.c-torture/execute/pr93744-2.c: New test. * gcc.c-torture/execute/pr93744-3.c: New test.
The releases/gcc-8 branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>: https://gcc.gnu.org/g:e24c48585721fc4122ae10282e32af49aff4c380 commit r8-10025-ge24c48585721fc4122ae10282e32af49aff4c380 Author: Jakub Jelinek <jakub@redhat.com> Date: Sat Feb 15 12:53:44 2020 +0100 match.pd: Disallow side-effects in GENERIC for non-COND_EXPR to COND_EXPR simplifications [PR93744] As the following testcases show (the first one reported, last two found by code inspection), we need to disallow side-effects in simplifications that turn some unconditional expression into conditional one. From my little understanding of genmatch.c, it is able to automatically disallow side effects if the same operand is used multiple times in the match pattern, maybe if it is used multiple times in the replacement pattern, and if it is used in conditional contexts in the match pattern, could it be taught to handle this case too? If yes, perhaps just the first hunk could be usable for 8/9 backports (+ the testcases). 2020-02-15 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/93744 * match.pd (((m1 >/</>=/<= m2) * d -> (m1 >/</>=/<= m2) ? d : 0, A - ((A - B) & -(C cmp D)) -> (C cmp D) ? B : A, A + ((B - A) & -(C cmp D)) -> (C cmp D) ? B : A): For GENERIC, make sure @2 in the first and @1 in the other patterns has no side-effects. * gcc.c-torture/execute/pr93744-1.c: New test. * gcc.c-torture/execute/pr93744-2.c: New test. * gcc.c-torture/execute/pr93744-3.c: New test.
Fixed for 8.4+/9.3+/10+.