diff mbox

Fix normal_inner_ref expansion (PR middle-end/69909)

Message ID 20160223154344.GB3017@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 23, 2016, 3:43 p.m. UTC
Hi!

When the base of a handled component (BIT_FIELD_REF in the testcase)
is SSA_NAME which happens to be expanded as some MEM (on the testcase
it is SSA_NAME set to VIEW_CONVERT_EXPR of an SSA_NAME that has MEM as
DECL_RTL), expand_expr_real_1 can try to update the MEM attributes from
exp, but that is wrong, it might change the alias set of the MEM, etc.
If the base is SSA_NAME, we should keep the attributes unmodified.
The patch actually also tests for !MEM_P (orig_op0), so that if
the SSA_NAME expanded to non-MEM (e.g. constant or REG), but the
normal_inner_ref expansion forces it for whatever reason in memory,
we still set the attributes of such a MEM, it is a temporary in that
case, rather than the original read.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-02-23  Jakub Jelinek  <jakub@redhat.com>
	    Richard Biener  <rguenth@suse.de>

	PR middle-end/69909
	* expr.c (expand_expr_real_1) <normal_inner_ref>: Avoid
	set_mem_attributes if tem is SSA_NAME which got expanded
	as a MEM.

	* gcc.dg/torture/pr69909.c: New test.


	Jakub

Comments

Richard Biener Feb. 24, 2016, 8:25 a.m. UTC | #1
On Tue, 23 Feb 2016, Jakub Jelinek wrote:

> Hi!
> 
> When the base of a handled component (BIT_FIELD_REF in the testcase)
> is SSA_NAME which happens to be expanded as some MEM (on the testcase
> it is SSA_NAME set to VIEW_CONVERT_EXPR of an SSA_NAME that has MEM as
> DECL_RTL), expand_expr_real_1 can try to update the MEM attributes from
> exp, but that is wrong, it might change the alias set of the MEM, etc.
> If the base is SSA_NAME, we should keep the attributes unmodified.
> The patch actually also tests for !MEM_P (orig_op0), so that if
> the SSA_NAME expanded to non-MEM (e.g. constant or REG), but the
> normal_inner_ref expansion forces it for whatever reason in memory,
> we still set the attributes of such a MEM, it is a temporary in that
> case, rather than the original read.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2016-02-23  Jakub Jelinek  <jakub@redhat.com>
> 	    Richard Biener  <rguenth@suse.de>
> 
> 	PR middle-end/69909
> 	* expr.c (expand_expr_real_1) <normal_inner_ref>: Avoid
> 	set_mem_attributes if tem is SSA_NAME which got expanded
> 	as a MEM.
> 
> 	* gcc.dg/torture/pr69909.c: New test.
> 
> --- gcc/expr.c.jj	2016-02-23 13:54:02.000000000 +0100
> +++ gcc/expr.c	2016-02-23 14:30:23.810657866 +0100
> @@ -10521,7 +10521,11 @@ expand_expr_real_1 (tree exp, rtx target
>  	if (op0 == orig_op0)
>  	  op0 = copy_rtx (op0);
>  
> -	set_mem_attributes (op0, exp, 0);
> +	/* Don't set memory attributes if the base expression is
> +	   SSA_NAME that got expanded as a MEM.  In that case, we should
> +	   just honor its original memory attributes.  */
> +	if (TREE_CODE (tem) != SSA_NAME || !MEM_P (orig_op0))
> +	  set_mem_attributes (op0, exp, 0);
>  
>  	if (REG_P (XEXP (op0, 0)))
>  	  mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0));
> --- gcc/testsuite/gcc.dg/torture/pr69909.c.jj	2016-02-23 14:25:27.819719259 +0100
> +++ gcc/testsuite/gcc.dg/torture/pr69909.c	2016-02-23 14:25:27.818719272 +0100
> @@ -0,0 +1,35 @@
> +/* PR middle-end/69909 */
> +/* { dg-do run { target int128 } } */
> +/* { dg-additional-options "-w" } */
> +
> +typedef unsigned V __attribute__ ((vector_size (32)));
> +typedef __int128 T;
> +typedef __int128 U __attribute__ ((vector_size (32)));
> +
> +__attribute__((noinline, noclone)) T
> +foo (T a, V b, V c, V d, V e, U f)
> +{
> +  d[6] ^= 0x10;
> +  f -= (U) d;
> +  f[1] |= f[1] << (a & 127);
> +  c ^= d;
> +  return b[7] + c[2] + c[2] + d[6] + e[2] + f[1];
> +}
> +
> +int
> +main ()
> +{
> +  if (__CHAR_BIT__ != 8 || sizeof (unsigned) != 4 || sizeof (T) != 16)
> +    return 0;
> +
> +  T x = foo (1, (V) { 9, 2, 5, 8, 1, 2, 9, 3 },
> +		(V) { 1, 2, 3, 4, 5, 6, 7, 8 },
> +		(V) { 4, 1, 2, 9, 8, 3, 5, 2 },
> +		(V) { 3, 6, 1, 3, 2, 9, 4, 8 }, (U) { 3, 5 });
> +  if (((unsigned long long) (x >> 64) != 0xffffffffffffffffULL
> +       || (unsigned long long) x != 0xfffffffe0000001aULL)
> +      && ((unsigned long long) (x >> 64) != 0xfffffffffffffffdULL
> +	  || (unsigned long long) x != 0xffffffff00000022ULL))
> +    __builtin_abort ();
> +  return 0;
> +}
> 
> 	Jakub
> 
>
diff mbox

Patch

--- gcc/expr.c.jj	2016-02-23 13:54:02.000000000 +0100
+++ gcc/expr.c	2016-02-23 14:30:23.810657866 +0100
@@ -10521,7 +10521,11 @@  expand_expr_real_1 (tree exp, rtx target
 	if (op0 == orig_op0)
 	  op0 = copy_rtx (op0);
 
-	set_mem_attributes (op0, exp, 0);
+	/* Don't set memory attributes if the base expression is
+	   SSA_NAME that got expanded as a MEM.  In that case, we should
+	   just honor its original memory attributes.  */
+	if (TREE_CODE (tem) != SSA_NAME || !MEM_P (orig_op0))
+	  set_mem_attributes (op0, exp, 0);
 
 	if (REG_P (XEXP (op0, 0)))
 	  mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0));
--- gcc/testsuite/gcc.dg/torture/pr69909.c.jj	2016-02-23 14:25:27.819719259 +0100
+++ gcc/testsuite/gcc.dg/torture/pr69909.c	2016-02-23 14:25:27.818719272 +0100
@@ -0,0 +1,35 @@ 
+/* PR middle-end/69909 */
+/* { dg-do run { target int128 } } */
+/* { dg-additional-options "-w" } */
+
+typedef unsigned V __attribute__ ((vector_size (32)));
+typedef __int128 T;
+typedef __int128 U __attribute__ ((vector_size (32)));
+
+__attribute__((noinline, noclone)) T
+foo (T a, V b, V c, V d, V e, U f)
+{
+  d[6] ^= 0x10;
+  f -= (U) d;
+  f[1] |= f[1] << (a & 127);
+  c ^= d;
+  return b[7] + c[2] + c[2] + d[6] + e[2] + f[1];
+}
+
+int
+main ()
+{
+  if (__CHAR_BIT__ != 8 || sizeof (unsigned) != 4 || sizeof (T) != 16)
+    return 0;
+
+  T x = foo (1, (V) { 9, 2, 5, 8, 1, 2, 9, 3 },
+		(V) { 1, 2, 3, 4, 5, 6, 7, 8 },
+		(V) { 4, 1, 2, 9, 8, 3, 5, 2 },
+		(V) { 3, 6, 1, 3, 2, 9, 4, 8 }, (U) { 3, 5 });
+  if (((unsigned long long) (x >> 64) != 0xffffffffffffffffULL
+       || (unsigned long long) x != 0xfffffffe0000001aULL)
+      && ((unsigned long long) (x >> 64) != 0xfffffffffffffffdULL
+	  || (unsigned long long) x != 0xffffffff00000022ULL))
+    __builtin_abort ();
+  return 0;
+}