Bug 93744

Summary: [8/9/10 Regression] Different results between gcc-9 and gcc-7
Product: gcc Reporter: Karine EM <k.even-mendoza>
Component: tree-optimizationAssignee: Jakub Jelinek <jakub>
Status: RESOLVED FIXED    
Severity: normal CC: amonakov, jakub
Priority: P2 Keywords: wrong-code
Version: 9.2.1   
Target Milestone: 8.4   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2020-02-14 00:00:00
Attachments: gcc10-pr93744.patch

Description Karine EM 2020-02-14 18:04:20 UTC
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.
Comment 1 Jakub Jelinek 2020-02-14 21:18:37 UTC
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;
}
Comment 2 Andrew Pinski 2020-02-14 21:23:02 UTC
(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.
Comment 3 Jakub Jelinek 2020-02-14 21:33:12 UTC
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 ;).
Comment 4 Alexander Monakov 2020-02-14 21:44:01 UTC
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.
Comment 5 Jakub Jelinek 2020-02-14 21:53:39 UTC
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;
}
Comment 6 Jakub Jelinek 2020-02-14 22:00:43 UTC
Created attachment 47844 [details]
gcc10-pr93744.patch

Untested fix.  I'd say issues with that pattern unrelated to side-effects should be tracked independently.
Comment 7 CVS Commits 2020-02-15 11:55:19 UTC
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.
Comment 8 Jakub Jelinek 2020-02-15 12:15:31 UTC
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.
Comment 9 CVS Commits 2020-02-15 12:23:07 UTC
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.
Comment 10 CVS Commits 2020-02-15 12:27:26 UTC
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.
Comment 11 Jakub Jelinek 2020-02-15 12:27:50 UTC
Fixed for 8.4+/9.3+/10+.