Bug 62030 - wrong code due to ifcvt merging two stores which have different aliasing sets
Summary: wrong code due to ifcvt merging two stores which have different aliasing sets
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 5.0
: P3 normal
Target Milestone: ---
Assignee: Tom de Vries
URL:
Keywords: alias, patch, wrong-code
Depends on:
Blocks: 61964
  Show dependency treegraph
 
Reported: 2014-08-06 05:08 UTC by Drea Pinski
Modified: 2014-08-18 08:38 UTC (History)
2 users (show)

See Also:
Host:
Target: mipsisa64-elf
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-08-08 00:00:00


Attachments
Updated tentative patch for if-conversion, including fix for pr62030 (1.52 KB, patch)
2014-08-06 09:35 UTC, Tom de Vries
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Drea Pinski 2014-08-06 05:08:03 UTC
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.
Comment 1 Drea Pinski 2014-08-06 05:29:40 UTC
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.
Comment 2 Tom de Vries 2014-08-06 06:58:09 UTC
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
...
Comment 3 Drea Pinski 2014-08-06 07:03:16 UTC
(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.
Comment 4 Tom de Vries 2014-08-06 07:43:13 UTC
(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.
Comment 5 Tom de Vries 2014-08-06 09:35:58 UTC
Created attachment 33258 [details]
Updated tentative patch for if-conversion, including fix for pr62030

Updated tentative patch for if-conversion, including fix for pr62030
Comment 7 Tom de Vries 2014-08-14 16:14:31 UTC
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
Comment 8 Tom de Vries 2014-08-14 17:49:17 UTC
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.
Comment 9 Tom de Vries 2014-08-15 21:23:54 UTC
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
Comment 10 Tom de Vries 2014-08-16 17:38:35 UTC
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
Comment 11 Tom de Vries 2014-08-18 08:38:20 UTC
Patch and test-cases (general and mips-specific) committed to trunk, 4.9 and 4.8.

Marking resolved fixed