[Bug tree-optimization/64853] New: wrong code at -Os and above on x86_64-linux-gnu

[Bug tree-optimization/64853] New: wrong code at -Os and above on x86_64-linux-gnu

[su at cs dot ucdavis.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

            Bug ID: 64853
           Summary: wrong code at -Os and above on x86_64-linux-gnu
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: su at cs dot ucdavis.edu

The current gcc trunk miscompiles the following code on x86_64-linux at -Os and
above in both 32-bit and 64-bit modes. 

This is a regression from 4.9.x. 


$ gcc-trunk -v
Using built-in specs.
COLLECT_GCC=gcc-trunk
COLLECT_LTO_WRAPPER=/usr/local/gcc-trunk/libexec/gcc/x86_64-unknown-linux-gnu/5.0.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with: ../gcc-trunk/configure --prefix=/usr/local/gcc-trunk
--enable-languages=c,c++ --disable-werror --enable-multilib
Thread model: posix
gcc version 5.0.0 20150128 (experimental) [trunk revision 220198] (GCC) 
$ 
$ gcc-trunk -O1 small.c; ./a.out
$ gcc-4.9 -Os small.c; ./a.out
$ 
$ gcc-trunk -Os small.c
$ ./a.out
Aborted (core dumped)
$ 



---------------------------------



struct S
{
  int f1;
};

static struct S a = { 1 };
char b;
static unsigned char *c = &b;
int d, e, f;

int
fn1 (int p)
{
  return 0 ? 0 : p - 1;
}

static int
fn2 (struct S p)
{
  int g = 200;
  for (e = 4; e; e = fn1 (e))
    {
      for (; d; d++)
    ;
      *c &= p.f1 & g;
      g = --*c;
      if (f)
    return 0;
    }
  return 0;
}

int
main ()
{
  fn2 (a);

  if (b != 0) 
    __builtin_abort (); 

  return 0;
}

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-01-29
                 CC|                            |jakub at gcc dot gnu.org
   Target Milestone|---                         |5.0
            Summary|wrong code at -Os and above |[5 Regression] wrong code
                   |on x86_64-linux-gnu         |at -Os and above on
                   |                            |x86_64-linux-gnu
     Ever confirmed|0                           |1

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
This started with r217560, the first difference is during VRP2:
 Visiting statement:
 _13 = (signed char) g_68;
+Match-and-simplified (signed char) g_68 to -56(OVF)
 Found new range for _13: [-56, -56]
and various others, leading to
-_10: ~[1, 254]
+_10: [255, +INF]
 .MEM_11: VARYING
-g_12: [0, 255]
-_13: VARYING
+g_12: [255, 255]
+_13: [-56, -1]
etc.  Haven't checked if the changes are correct or not.

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think the problem is that:


Visiting statement:
_15 = _56 & 1;
Applying pattern match.pd:312, gimple-match.c:13772
Applying pattern match.pd:235, gimple-match.c:13525
Match-and-simplified _56 & 1 to 0
Intersecting
  [0, 0]
and
  [0, 1]
to
  [0, 0]
Found new range for _15: [0, 0]


might be reasonable during iteration, when not yet considering the backedge as
executable, but we actually never visit this statement again, perhaps due to
match-and-simplify looking at earlier statements.  Richard, can you please have
a look?

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Simulating block 2
Simulating block 2
Simulating block 25
Simulating block 15
Simulating block 4
Simulating block 3
Simulating block 5
Simulating block 6
Simulating block 4
Simulating block 12
Simulating block 13
Simulating block 26
Simulating block 14
Simulating block 8
Simulating block 7
Simulating block 15
Simulating block 16
Simulating block 17
Simulating block 10
Simulating block 8
Simulating block 9
Simulating block 18
Simulating block 19
Simulating block 23
Simulating block 11
Simulating block 21
Simulating block 10
Simulating block 20
Simulating block 22
Simulating block 11

So, we indeed simulate block 5 only the first time after simulating block 15
(i.e. when edge 25 -> 15 is executable and 14 -> 15 is not), and never again
(block 15 itself is simulated again with both incoming edges executable).

Wonder if for match-and-simplify during the VRP folding that looks at other
statements we shouldn't somehow note at which statements we've actually looked
when making the decision (but how?) and mark the statement for resimulation.

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
--- gcc/tree-vrp.c.jj    2015-01-28 08:39:51.000000000 +0100
+++ gcc/tree-vrp.c    2015-01-29 10:44:37.395688127 +0100
@@ -7093,14 +7093,14 @@ vrp_valueize_1 (tree name)
   if (TREE_CODE (name) == SSA_NAME)
     {
       value_range_t *vr = get_value_range (name);
-      if (range_int_cst_singleton_p (vr))
-    return vr->min;
       /* If the definition may be simulated again we cannot follow
          this SSA edge as the SSA propagator does not necessarily
      re-visit the use.  */
       gimple def_stmt = SSA_NAME_DEF_STMT (name);
       if (prop_simulate_again_p (def_stmt))
     return NULL_TREE;
+      if (range_int_cst_singleton_p (vr))
+    return vr->min;
     }
   return name;
 }

fixes this, even when a VR is singleton, it might be singleton just because it
is singleton only in the first iteration and not afterwards.
That said, I'd worry that this patch might degrade VRP folding too much.

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Mine.  I thought I had handled this correctly with vrp_valueize_1 doing

/* Return the singleton value-range for NAME if that is a constant
   but signal to not follow SSA edges.  */

static inline tree
vrp_valueize_1 (tree name)
{
  if (TREE_CODE (name) == SSA_NAME)
    {
      value_range_t *vr = get_value_range (name);
      if (range_int_cst_singleton_p (vr))
        return vr->min;
      /* If the definition may be simulated again we cannot follow
         this SSA edge as the SSA propagator does not necessarily
         re-visit the use.  */
      gimple def_stmt = SSA_NAME_DEF_STMT (name);
      if (prop_simulate_again_p (def_stmt))
^^^
        return NULL_TREE;

but I guess the singleton handling needs to be guarded with the same.

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
I have a patch (CCP is also affected).

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Sth like

Index: gcc/tree-ssa-propagate.c
===================================================================
--- gcc/tree-ssa-propagate.c    (revision 220235)
+++ gcc/tree-ssa-propagate.c    (working copy)
@@ -364,6 +364,7 @@ simulate_stmt (gimple stmt)
          FOR_EACH_EDGE (e, ei, bb->succs)
            add_control_edge (e);
        }
+      return;
     }
   else if (val == SSA_PROP_INTERESTING)
     {
@@ -377,6 +378,45 @@ simulate_stmt (gimple stmt)
       if (taken_edge)
        add_control_edge (taken_edge);
     }
+
+  /* If there are no SSA uses on the stmt whose defs are simulated
+     again then this stmt will be never visited again.  */
+  bool has_simulate_again_uses = false;
+  use_operand_p use_p;
+  ssa_op_iter iter;
+  if (gimple_code  (stmt) == GIMPLE_PHI)
+    {
+      edge_iterator ei;
+      edge e;
+      tree arg;
+      FOR_EACH_EDGE (e, ei, gimple_bb (stmt)->preds)
+       if (!(e->flags & EDGE_EXECUTABLE)
+           || ((arg = PHI_ARG_DEF_FROM_EDGE (stmt, e))
+               && TREE_CODE (arg) == SSA_NAME
+               && !SSA_NAME_IS_DEFAULT_DEF (arg)
+               && prop_simulate_again_p (SSA_NAME_DEF_STMT (arg))))
+         {
+           has_simulate_again_uses = true;
+           break;
+         }
+    }
+  else
+    FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_USE)
+      {
+       gimple def_stmt = SSA_NAME_DEF_STMT (USE_FROM_PTR (use_p));
+       if (!gimple_nop_p (def_stmt)
+           && prop_simulate_again_p (def_stmt))
+         {
+           has_simulate_again_uses = true;
+           break;
+         }
+      }
+  if (!has_simulate_again_uses)
+    {
+      if (dump_file && (dump_flags & TDF_DETAILS))
+       fprintf (dump_file, "marking stmt to be not simulated again\n");
+      prop_set_simulate_again (stmt, false);
+    }
 }

 /* Process an SSA edge worklist.  WORKLIST is the SSA edge worklist to

for what we discussed on IRC.  Probably makes sense to omit PHI handling
first as that's definitely "interesting".  I'll bootstrap that separately
on the fix for this PR (without handling PHIs).

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.

[Bug tree-optimization/64853] [5 Regression] wrong code at -Os and above on x86_64-linux-gnu

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64853

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Author: rguenth
Date: Thu Jan 29 13:50:37 2015
New Revision: 220247

URL: https://gcc.gnu.org/viewcvs?rev=220247&root=gcc&view=rev
Log:
2015-01-29  Richard Biener  <rguenther@suse.de>

    PR tree-optimization/64853
    * tree-vrp.c (vrp_valueize_1): Do not return anything if the
    stmt will get simulated again.
    * tree-ssa-ccp.c (valueize_op_1): Likewise.

    * gcc.dg/torture/pr64853.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr64853.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-ccp.c
    trunk/gcc/tree-vrp.c