diff mbox

Fix if-conversion pass for dead type-unsafe code

Message ID 53E4B4DC.3030901@mentor.com
State New
Headers show

Commit Message

Tom de Vries Aug. 8, 2014, 11:30 a.m. UTC
Steven,

this patch fixes:
- PR62004 (the if-conversion pass part, the tail-merge part is still todo), and
- PR62030.

In both cases, a valid program with a dead type-unsafe access is transformed by 
the if-conversion pass into an invalid program with a live type-unsafe access.

The transformation done by the if-conversion pass that suffers from this problem 
is if-merging, replacing the if-then-else with the if-block or the then then-block.

The patch fixes this problem by detecting when the if-block and the then-block 
are treated differently by alias analysis, and preventing the optimization in 
that case.

This part of the patch fixes PR62004.
...
@@ -2583,7 +2631,7 @@ noce_process_if_block (struct noce_if_info *if_info)

    /* Look and see if A and B are really the same.  Avoid creating silly
       cmove constructs that no one will fix up later.  */
-  if (rtx_equal_p (a, b))
+  if (rtx_interchangeable_p (a, b))
      {
        /* If we have an INSN_B, we don't have to create any new rtl.  Just
	 move the instruction that we already have.  If we don't have an
...

This part of the patch fixes PR62030:
...
@@ -2517,7 +2565,7 @@ noce_process_if_block (struct noce_if_info *if_info)
	  || BLOCK_FOR_INSN (insn_b) != BLOCK_FOR_INSN (if_info->cond_earliest)
	  || !NONJUMP_INSN_P (insn_b)
	  || (set_b = single_set (insn_b)) == NULL_RTX
-         || ! rtx_equal_p (x, SET_DEST (set_b))
+         || ! rtx_interchangeable_p (x, SET_DEST (set_b))
	  || ! noce_operand_ok (SET_SRC (set_b))
	  || reg_overlap_mentioned_p (x, SET_SRC (set_b))
	  || modified_between_p (SET_SRC (set_b), insn_b, jump)
...

I've added the other fixes after review of the if-conversion pass for the same 
problem, I hope this is complete (well, at least for the if-conversion pass. I 
wonder if cross-jumping suffers from the same problem).

The PR62030 test-case fails with trunk on MIPS. The PR62004 testcase fails with 
4.8 on x86_64. But I think the problem exists in trunk, 4.9 and 4.8, it's just 
hard to trigger.

Bootstrapped and reg-tested on x86_64, trunk. No issue found other than 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62060 , which looks like a 
test-case issue.

OK for trunk, 4.9, 4.8?

Thanks,
- Tom

Comments

Richard Biener Aug. 8, 2014, 11:36 a.m. UTC | #1
On Fri, 8 Aug 2014, Tom de Vries wrote:

> Steven,
> 
> this patch fixes:
> - PR62004 (the if-conversion pass part, the tail-merge part is still todo),
> and
> - PR62030.
> 
> In both cases, a valid program with a dead type-unsafe access is transformed
> by the if-conversion pass into an invalid program with a live type-unsafe
> access.
> 
> The transformation done by the if-conversion pass that suffers from this
> problem is if-merging, replacing the if-then-else with the if-block or the
> then then-block.
> 
> The patch fixes this problem by detecting when the if-block and the then-block
> are treated differently by alias analysis, and preventing the optimization in
> that case.
> 
> This part of the patch fixes PR62004.
> ...
> @@ -2583,7 +2631,7 @@ noce_process_if_block (struct noce_if_info *if_info)
> 
>    /* Look and see if A and B are really the same.  Avoid creating silly
>       cmove constructs that no one will fix up later.  */
> -  if (rtx_equal_p (a, b))
> +  if (rtx_interchangeable_p (a, b))
>      {
>        /* If we have an INSN_B, we don't have to create any new rtl.  Just
> 	 move the instruction that we already have.  If we don't have an
> ...
> 
> This part of the patch fixes PR62030:
> ...
> @@ -2517,7 +2565,7 @@ noce_process_if_block (struct noce_if_info *if_info)
> 	  || BLOCK_FOR_INSN (insn_b) != BLOCK_FOR_INSN
> (if_info->cond_earliest)
> 	  || !NONJUMP_INSN_P (insn_b)
> 	  || (set_b = single_set (insn_b)) == NULL_RTX
> -         || ! rtx_equal_p (x, SET_DEST (set_b))
> +         || ! rtx_interchangeable_p (x, SET_DEST (set_b))
> 	  || ! noce_operand_ok (SET_SRC (set_b))
> 	  || reg_overlap_mentioned_p (x, SET_SRC (set_b))
> 	  || modified_between_p (SET_SRC (set_b), insn_b, jump)
> ...
> 
> I've added the other fixes after review of the if-conversion pass for the same
> problem, I hope this is complete (well, at least for the if-conversion pass. I
> wonder if cross-jumping suffers from the same problem).
> 
> The PR62030 test-case fails with trunk on MIPS. The PR62004 testcase fails
> with 4.8 on x86_64. But I think the problem exists in trunk, 4.9 and 4.8, it's
> just hard to trigger.
> 
> Bootstrapped and reg-tested on x86_64, trunk. No issue found other than
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62060 , which looks like a
> test-case issue.
> 
> OK for trunk, 4.9, 4.8?

Maybe instead of a new mem_alias_equal_p simply compare MEM_ATTRs
with mem_attrs_eq_p?  Note that

+  if (flag_strict_aliasing
+      && MEM_ALIAS_SET (a) != MEM_ALIAS_SET (b))
+    return false;

looks wrong as it doesn't treat ALIAS_SET_MEMORY_BARRIER specially.
Also note that PRE already does sth similar with using exp_equiv_p
and passing 'true' for the 'for_gcse' argument.  In fact
using exp_equiv_p would likely improve if-conversion if used
in place of rtx_equal_p?

Thanks,
Richard.
diff mbox

Patch

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

	* ifcvt.c (mem_alias_equal_p, rtx_interchangeable_p): New function.
	(noce_try_move, noce_process_if_block): Use rtx_interchangeable_p.
	(noce_try_cmove_arith): Use mem_alias_equal_p.

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

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index faf9b30..6da3e0f 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -306,6 +306,52 @@  block_fallthru (basic_block bb)
 
   return (e) ? e->dest : NULL_BLOCK;
 }
+
+/* Return true if MEMs A and B are treated equal by alias analysis.  */
+
+static bool
+mem_alias_equal_p (const_rtx a, const_rtx b)
+{
+  gcc_assert (GET_CODE (a) == MEM && GET_CODE (b) == MEM);
+  
+  if (MEM_ADDR_SPACE (a) != MEM_ADDR_SPACE (b))
+    return false;
+
+  if (flag_strict_aliasing
+      && MEM_ALIAS_SET (a) != MEM_ALIAS_SET (b))
+    return false;
+
+  if (MEM_EXPR (a) != MEM_EXPR (b))
+    return false;
+
+  if (MEM_OFFSET_KNOWN_P (a) != MEM_OFFSET_KNOWN_P (b))
+    return false;
+
+  if (MEM_OFFSET_KNOWN_P (a)
+      && MEM_OFFSET (a) != MEM_OFFSET (b))
+    return false;
+
+  return true;
+}
+
+static bool
+rtx_interchangeable_p (const_rtx a, const_rtx b)
+{
+  if (!rtx_equal_p (a, b))
+    return false;
+
+  if (GET_CODE (a) != MEM)
+    return true;
+  
+  /* A dead type-unsafe memory reference is legal, but a live type-unsafe memory
+     reference is not.  Interchanging a dead type-unsafe memory reference with
+     a live type-safe one creates a live type-unsafe memory reference, in other
+     words, it makes the program illegal.  So we check whether the two memory
+     references have equal tbaa properties.  */
+
+  return mem_alias_equal_p (a, b);
+}
+
 
 /* Go through a bunch of insns, converting them to conditional
    execution format if possible.  Return TRUE if all of the non-note
@@ -1034,6 +1080,9 @@  noce_try_move (struct noce_if_info *if_info)
       || (rtx_equal_p (if_info->a, XEXP (cond, 1))
 	  && rtx_equal_p (if_info->b, XEXP (cond, 0))))
     {
+      if (!rtx_interchangeable_p (if_info->a, if_info->b))
+	return FALSE;
+
       y = (code == EQ) ? if_info->a : if_info->b;
 
       /* Avoid generating the move if the source is the destination.  */
@@ -1537,14 +1586,13 @@  noce_try_cmove_arith (struct noce_if_info *if_info)
   int insn_cost;
   enum rtx_code code;
 
-  /* A conditional move from two memory sources is equivalent to a
-     conditional on their addresses followed by a load.  Don't do this
-     early because it'll screw alias analysis.  Note that we've
-     already checked for no side effects.  */
-  /* ??? FIXME: Magic number 5.  */
+  /* A conditional move from two memory sources is equivalent to a conditional
+     on their addresses followed by a load.  Don't do this if it'll screw alias
+     analysis.  Note that we've already checked for no side effects.  */
   if (cse_not_expected
       && MEM_P (a) && MEM_P (b)
-      && MEM_ADDR_SPACE (a) == MEM_ADDR_SPACE (b)
+      && mem_alias_equal_p (a, b)
+      /* ??? FIXME: Magic number 5.  */
       && if_info->branch_cost >= 5)
     {
       enum machine_mode address_mode = get_address_mode (a);
@@ -2504,7 +2552,7 @@  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_interchangeable_p (x, SET_DEST (set_b)))
 	return FALSE;
     }
   else
@@ -2517,7 +2565,7 @@  noce_process_if_block (struct noce_if_info *if_info)
 	  || BLOCK_FOR_INSN (insn_b) != BLOCK_FOR_INSN (if_info->cond_earliest)
 	  || !NONJUMP_INSN_P (insn_b)
 	  || (set_b = single_set (insn_b)) == NULL_RTX
-	  || ! rtx_equal_p (x, SET_DEST (set_b))
+	  || ! rtx_interchangeable_p (x, SET_DEST (set_b))
 	  || ! noce_operand_ok (SET_SRC (set_b))
 	  || reg_overlap_mentioned_p (x, SET_SRC (set_b))
 	  || modified_between_p (SET_SRC (set_b), insn_b, jump)
@@ -2583,7 +2631,7 @@  noce_process_if_block (struct noce_if_info *if_info)
 
   /* Look and see if A and B are really the same.  Avoid creating silly
      cmove constructs that no one will fix up later.  */
-  if (rtx_equal_p (a, b))
+  if (rtx_interchangeable_p (a, b))
     {
       /* If we have an INSN_B, we don't have to create any new rtl.  Just
 	 move the instruction that we already have.  If we don't have an
diff --git a/gcc/testsuite/gcc.dg/pr62004.c b/gcc/testsuite/gcc.dg/pr62004.c
new file mode 100644
index 0000000..7292c4f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr62004.c
@@ -0,0 +1,47 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-tree-tail-merge" } */
+
+struct node
+{ 
+  struct node *next; 
+  struct node *prev; 
+};
+
+struct node node;
+
+struct head
+{
+  struct node *first;
+};
+
+struct head heads[5];
+
+int k = 2;
+
+struct head *head = &heads[2];
+
+int
+main ()
+{
+  struct node *p;
+
+  node.next = (void*)0;
+
+  node.prev = (void *)head;
+
+  head->first = &node;
+
+  struct node *n = head->first;
+
+  struct head *h = &heads[k];
+
+  heads[2].first = n->next;
+
+  if ((void*)n->prev == (void *)h)
+    p = h->first;
+  else
+    /* Dead tbaa-unsafe load from ((struct node *)&heads[2])->next.  */
+    p = n->prev->next;
+
+  return !(p == (void*)0);
+}
diff --git a/gcc/testsuite/gcc.dg/pr62030.c b/gcc/testsuite/gcc.dg/pr62030.c
new file mode 100644
index 0000000..a16a970
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr62030.c
@@ -0,0 +1,50 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+extern void abort (void);
+
+struct node
+{ 
+  struct node *next; 
+  struct node *prev; 
+};
+
+struct node node;
+
+struct head
+{
+  struct node *first;
+};
+
+struct head heads[5];
+
+int k = 2;
+
+struct head *head = &heads[2];
+
+static int __attribute__((noinline))
+foo (void)
+{
+  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 (void)
+{
+  if (foo ())
+    abort ();
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.target/mips/pr62030-octeon.c b/gcc/testsuite/gcc.target/mips/pr62030-octeon.c
new file mode 100644
index 0000000..2f86f5e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/pr62030-octeon.c
@@ -0,0 +1,50 @@ 
+/* { dg-do run } */
+/* { dg-options "-march=octeon" } */
+
+extern void abort (void);
+
+struct node
+{ 
+  struct node *next; 
+  struct node *prev; 
+};
+
+struct node node;
+
+struct head
+{
+  struct node *first;
+};
+
+struct head heads[5];
+
+int k = 2;
+
+struct head *head = &heads[2];
+
+static int __attribute__((noinline))
+foo (void)
+{
+  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 (void)
+{
+  if (foo ())
+    abort ();
+  return 0;
+}
-- 
1.9.1