Re: Fix if-conversion pass for dead type-unsafe code

[Tom de Vries <Tom_deVries@mentor.com>]

[-- Attachment #1: Type: text/plain, Size: 5028 bytes --]

On 08-08-14 13:36, Richard Biener wrote:
> 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.

Hmm, I see now that in noce_try_cmove_arith a new mem is generated, and merging 
of (some of the) mem attributes is done, so that part doesn't need fixing for 
4.8/4.9:
...
   /* If we're handling a memory for above, emit the load now.  */
   if (is_mem)
     {
       tmp = gen_rtx_MEM (GET_MODE (if_info->x), target);

       /* Copy over flags as appropriate.  */
       if (MEM_VOLATILE_P (if_info->a) || MEM_VOLATILE_P (if_info->b))
         MEM_VOLATILE_P (tmp) = 1;
       if (MEM_ALIAS_SET (if_info->a) == MEM_ALIAS_SET (if_info->b))
         set_mem_alias_set (tmp, MEM_ALIAS_SET (if_info->a));
       set_mem_align (tmp,
                      MIN (MEM_ALIGN (if_info->a), MEM_ALIGN (if_info->b)));

       gcc_assert (MEM_ADDR_SPACE (if_info->a) == MEM_ADDR_SPACE (if_info->b));
       set_mem_addr_space (tmp, MEM_ADDR_SPACE (if_info->a));

       noce_emit_move_insn (if_info->x, tmp);
     }
...
Dropped in attached patch.

>> I
>> wonder if cross-jumping suffers from the same problem).
>>

Cross-jumping uses merge_memattrs.

>> 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?

I propose to fix it this way (as attached) on 4.8/4.9/trunk, and maybe do a more 
efficient handling on trunk as a follow-up patch.

I'll put this through bootstrap/test again.

>  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.

Ah, I see.

> 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?
>

Interesting. I've noticed btw that exp_equiv_p does not use mem_attrs_eq_p, but 
a mem-attrs pointer-equal comparison:
...
	  /* Can't merge two expressions in different alias sets, since we
	     can decide that the expression is transparent in a block when
	     it isn't, due to it being set with the different alias set.

	     Also, can't merge two expressions with different MEM_ATTRS.
	     They could e.g. be two different entities allocated into the
	     same space on the stack (see e.g. PR25130).  In that case, the
	     MEM addresses can be the same, even though the two MEMs are
	     absolutely not equivalent.

	     But because really all MEM attributes should be the same for
	     equivalent MEMs, we just use the invariant that MEMs that have
	     the same attributes share the same mem_attrs data structure.  */
	  if (MEM_ATTRS (x) != MEM_ATTRS (y))
	    return 0;
...

Thanks,
- Tom


[-- Attachment #2: 0001-Conservative-fix-for-PR62004-PR62030.patch --]
[-- Type: text/x-patch, Size: 6642 bytes --]

2014-08-08  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.

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index b5278bf..1cac518 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -263,7 +263,7 @@ mem_attrs_htab_hash (const void *x)
 
 /* Return true if the given memory attributes are equal.  */
 
-static bool
+bool
 mem_attrs_eq_p (const struct mem_attrs *p, const struct mem_attrs *q)
 {
   return (p->alias == q->alias
diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
index 7268090..9ccee07 100644
--- a/gcc/emit-rtl.h
+++ b/gcc/emit-rtl.h
@@ -69,6 +69,7 @@ extern void set_reg_attrs_for_parm (rtx, rtx);
 extern void set_reg_attrs_for_decl_rtl (tree t, rtx x);
 extern void adjust_reg_mode (rtx, enum machine_mode);
 extern int mem_expr_equal_p (const_tree, const_tree);
+extern bool mem_attrs_eq_p (const struct mem_attrs *, const struct mem_attrs *);
 
 extern bool need_atomic_barrier_p (enum memmodel, bool);
 
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index e3353a5..dd6df5b 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -294,6 +294,28 @@ block_fallthru (basic_block bb)
 
   return (e) ? e->dest : NULL_BLOCK;
 }
+
+/* Return true if RTXs A and B can be safely interchanged.  */
+
+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.
+     We check here conservatively whether the two memory references have equal
+     memory attributes.  */
+
+  return mem_attrs_eq_p (get_mem_attrs (a), get_mem_attrs (b));
+}
+
 \f
 /* Go through a bunch of insns, converting them to conditional
    execution format if possible.  Return TRUE if all of the non-note
@@ -1014,6 +1036,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.  */
@@ -2483,7 +2508,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
@@ -2496,7 +2521,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)
@@ -2562,7 +2587,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..c994a41
--- /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..b8baf93
--- /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..5e3d3b3
--- /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