[Bug rtl-optimization/62030] New: wrong code due to aliasing issue

[Bug rtl-optimization/62030] New: wrong code due to aliasing issue

[pinskia at gcc dot gnu.org]

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

            Bug ID: 62030
           Summary: wrong code due to aliasing issue
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Keywords: alias, wrong-code
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: pinskia at gcc dot gnu.org
            Blocks: 61964
            Target: mipsisa64-elf

With a slightly modified version of the testcase for PR 61964 (I have an
optimization pass which does the optimization but I can confirm it with an
upstream GCC with the modified source), I get the failure on mipsisa64-elf.
Here is the testcase:
/* { dg-do run } */

extern void abort (void);

struct node { struct node *next, *prev; } node;
struct head { struct node *first; } heads[5];
int k = 2;
struct head *head = &heads[2];

static int __attribute__((noinline))
foo()
{
  node.prev = (void *)head;
  head->first = &node;

  struct node *n = head->first;
  struct head *h = &heads[k];
  struct node *next = n->next;

  if (n->prev == (void *)h)
    h->first = next;
  else
    n->prev->next = next;

  n->next = h->first;
  return n->next == &node;
}

int main()
{
  if (foo ())
    abort ();
  return 0;
}

Compile with -O2 -march=octeon to see the failure.

CE1 is where the combining of the two stores happen.

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Here are the two stores:

(insn 30 25 33 3 (set (mem/f:DI (reg/v/f:DI 200 [ prev ]) [5 MEM[(struct head
*)&heads][_8].first+0 S8 A64])
        (reg/v/f:DI 199 [ next ])) t.c:22 302 {*movdi_64bit}
     (expr_list:REG_DEAD (reg/v/f:DI 200 [ prev ])
        (expr_list:REG_DEAD (reg/v/f:DI 199 [ next ])
            (nil))))


(insn 35 34 36 4 (set (mem/f:DI (reg/v/f:DI 200 [ prev ]) [3 prev_11->next+0 S8
A64])
        (reg/v/f:DI 199 [ next ])) t.c:24 302 {*movdi_64bit}
     (expr_list:REG_DEAD (reg/v/f:DI 200 [ prev ])
        (expr_list:REG_DEAD (reg/v/f:DI 199 [ next ])
            (nil))))


Note the reason why I think this does not happen for x86 (or even aarch64) is
due to the constraints on mem operands on MIPS.

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

vries at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vries at gcc dot gnu.org

--- Comment #2 from vries at gcc dot gnu.org ---
I think the test-case is reading an undefined value from n->next, but that's
easy enough to fix with an intializer for node.

Taking the tentative patch from PR62004, (
https://gcc.gnu.org/bugzilla/attachment.cgi?id=33242&action=diff ), with this
patch added prevents the if-conversion in this case:
...
@@ -2504,7 +2534,9 @@ noce_process_if_block (struct noce_if_info *if_info)
       if (! insn_b
      || insn_b != last_active_insn (else_bb, FALSE)
      || (set_b = single_set (insn_b)) == NULL_RTX
-         || ! rtx_equal_p (x, SET_DEST (set_b)))
+         || ! (rtx_equal_p (x, SET_DEST (set_b))
+               && (GET_CODE (x) != MEM
+                   || mem_interchangeable_p (x, SET_DEST (set_b))))
    return FALSE;
     }
   else
...

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to vries from comment #2)
> I think the test-case is reading an undefined value from n->next, but that's
> easy enough to fix with an intializer for node.

Since node is a global variable, it is zero initialized so there is no reading
from an uninitialized area.

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

--- Comment #4 from vries at gcc dot gnu.org ---
(In reply to Andrew Pinski from comment #3)
> (In reply to vries from comment #2)
> > I think the test-case is reading an undefined value from n->next, but that's
> > easy enough to fix with an intializer for node.
> 
> Since node is a global variable, it is zero initialized so there is no
> reading from an uninitialized area.

Ah, right, I thought that only applied to static. Thanks for setting that
straight.

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

--- Comment #5 from vries at gcc dot gnu.org ---
Created attachment 33258
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33258&action=edit
Updated tentative patch for if-conversion, including fix for pr62030

Updated tentative patch for if-conversion, including fix for pr62030

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

vries at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch
           Assignee|unassigned at gcc dot gnu.org      |vries at gcc dot gnu.org

--- Comment #6 from vries at gcc dot gnu.org ---
https://gcc.gnu.org/ml/gcc-patches/2014-08/msg00891.html

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

vries at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-08-08
     Ever confirmed|0                           |1

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

--- Comment #7 from vries at gcc dot gnu.org ---
Author: vries
Date: Thu Aug 14 16:13:59 2014
New Revision: 213970

URL: https://gcc.gnu.org/viewcvs?rev=213970&root=gcc&view=rev
Log:
Fix if-conversion pass for dead type-unsafe code

2014-08-14  Tom de Vries  <tom@codesourcery.com>

    PR rtl-optimization/62004
    PR rtl-optimization/62030
    * ifcvt.c (rtx_interchangeable_p): New function.
    (noce_try_move, noce_process_if_block): Use rtx_interchangeable_p.
    * emit-rtl.c (mem_attrs_eq_p): Remove static.
    * emit-rtl.h (mem_attrs_eq_p): Declare.

    * gcc.dg/pr62004.c: New test.
    * gcc.dg/pr62030.c: Same.
    * gcc.target/mips/pr62030-octeon.c: Same.

Added:
    trunk/gcc/testsuite/gcc.dg/pr62004.c
    trunk/gcc/testsuite/gcc.dg/pr62030.c
    trunk/gcc/testsuite/gcc.target/mips/pr62030-octeon.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/emit-rtl.h
    trunk/gcc/ifcvt.c
    trunk/gcc/testsuite/ChangeLog

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

--- Comment #8 from vries at gcc dot gnu.org ---
Author: vries
Revision: 213970
Modified property: svn:log

Modified: svn:log at Thu Aug 14 17:48:43 2014
------------------------------------------------------------------------------
--- svn:log (original)
+++ svn:log Thu Aug 14 17:48:43 2014
@@ -6,7 +6,6 @@
     PR rtl-optimization/62030
     * ifcvt.c (rtx_interchangeable_p): New function.
     (noce_try_move, noce_process_if_block): Use rtx_interchangeable_p.
-    * emit-rtl.c (mem_attrs_eq_p): Remove static.
     * emit-rtl.h (mem_attrs_eq_p): Declare.

     * gcc.dg/pr62004.c: New test.

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

--- Comment #9 from vries at gcc dot gnu.org ---
Author: vries
Date: Fri Aug 15 21:23:21 2014
New Revision: 214044

URL: https://gcc.gnu.org/viewcvs?rev=214044&root=gcc&view=rev
Log:
Fix if-conversion pass for dead type-unsafe code

2014-08-15  Tom de Vries  <tom@codesourcery.com>

    Backport from mainline:
    2014-08-14  Tom de Vries  <tom@codesourcery.com>

    PR rtl-optimization/62004
    PR rtl-optimization/62030
    * ifcvt.c (rtx_interchangeable_p): New function.
    (noce_try_move, noce_process_if_block): Use rtx_interchangeable_p.

    * gcc.dg/pr62004.c: New test.
    * gcc.dg/pr62030.c: Same.
    * gcc.target/mips/pr62030-octeon.c: Same.

    2014-08-05  Richard Biener  <rguenther@suse.de>

    * emit-rtl.h (mem_attrs_eq_p): Declare.
    * emit-rtl.c (mem_attrs_eq_p): Export.

Added:
    branches/gcc-4_9-branch/gcc/testsuite/gcc.dg/pr62004.c
    branches/gcc-4_9-branch/gcc/testsuite/gcc.dg/pr62030.c
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/mips/pr62030-octeon.c
Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/emit-rtl.c
    branches/gcc-4_9-branch/gcc/emit-rtl.h
    branches/gcc-4_9-branch/gcc/ifcvt.c
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

--- Comment #10 from vries at gcc dot gnu.org ---
Author: vries
Date: Sat Aug 16 17:38:04 2014
New Revision: 214067

URL: https://gcc.gnu.org/viewcvs?rev=214067&root=gcc&view=rev
Log:
Fix if-conversion pass for dead type-unsafe code

2014-08-15  Tom de Vries  <tom@codesourcery.com>

    Backport from mainline:
    2014-08-14  Tom de Vries  <tom@codesourcery.com>

    PR rtl-optimization/62004
    PR rtl-optimization/62030
    * ifcvt.c (rtx_interchangeable_p): New function.
    (noce_try_move, noce_process_if_block): Use rtx_interchangeable_p.

    * gcc.dg/pr62004.c: New test.
    * gcc.dg/pr62030.c: Same.
    * gcc.target/mips/pr62030-octeon.c: Same.

    2014-08-05  Richard Biener  <rguenther@suse.de>

    * emit-rtl.h (mem_attrs_eq_p): Declare.
    * emit-rtl.c (mem_attrs_eq_p): Export.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/pr62004.c
    branches/gcc-4_8-branch/gcc/testsuite/gcc.dg/pr62030.c
    branches/gcc-4_8-branch/gcc/testsuite/gcc.target/mips/pr62030-octeon.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/emit-rtl.c
    branches/gcc-4_8-branch/gcc/emit-rtl.h
    branches/gcc-4_8-branch/gcc/ifcvt.c
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog

[Bug rtl-optimization/62030] wrong code due to ifcvt merging two stores which have different aliasing sets

[vries at gcc dot gnu.org]

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

vries at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from vries at gcc dot gnu.org ---
Patch and test-cases (general and mips-specific) committed to trunk, 4.9 and
4.8.

Marking resolved fixed