rs6000 PR83660 fix ICE with vec_extract

Message ID 1523651828.6245.26.camel@linux.vnet.ibm.com
State New
Headers show
Series
  • rs6000 PR83660 fix ICE with vec_extract
Related show

Commit Message

Aaron Sawdey April 13, 2018, 8:37 p.m.
Per the discussion on the 83660, I've come to a minimal patch to
prevent this. Basically marking the vec_extract tree as having side
effects later makes sure that it gets all the cleanup points it needs
so that gimplify_cleanup_point_expr () is happy.  Also because
vec_insert puts a MODIFY_EXPR in there, it has side effects and this
problem will not occur.

Doing bootstrap/regtest on ppc64le with -mcpu=power7 since that is
where this issue arises. OK for trunk if everything passes?

Thanks,
   Aaron


2018-04-13  Aaron Sawdey  <acsawdey@linux.ibm.com>

	PR target/83660
	* config/rs6000/rs6000-c.c (altivec_resolve_overloaded_builtin): Mark
	vec_extract expression as having side effects to make sure it gets
	a cleanup point.

2018-04-13  Aaron Sawdey  <acsawdey@linux.ibm.com>

	PR target/83660
	* gcc.target/powerpc/pr83660.C: New test.

Comments

Segher Boessenkool April 13, 2018, 11:25 p.m. | #1
Hi!

On Fri, Apr 13, 2018 at 03:37:08PM -0500, Aaron Sawdey wrote:
> Per the discussion on the 83660, I've come to a minimal patch to
> prevent this. Basically marking the vec_extract tree as having side
> effects later makes sure that it gets all the cleanup points it needs
> so that gimplify_cleanup_point_expr () is happy.  Also because
> vec_insert puts a MODIFY_EXPR in there, it has side effects and this
> problem will not occur.
> 
> Doing bootstrap/regtest on ppc64le with -mcpu=power7 since that is
> where this issue arises. OK for trunk if everything passes?

> --- testsuite/gcc.target/powerpc/pr83660.C	(nonexistent)
> +++ testsuite/gcc.target/powerpc/pr83660.C	(working copy)
> @@ -0,0 +1,14 @@
> +/* PR target/83660 */
> +/* { dg-do compile } */
> +/* { dg-options "-mcpu=power7" } */

You'll need to skip this test if the user overrides -mcpu=; see the many
other testcases that do this.

With that fixed, okay for trunk.  Thanks!


Segher
Aaron Sawdey April 23, 2018, 4:36 p.m. | #2
This also affects gcc 7 and is fixed by the same patch. I've tested the
backport to 7 on ppc64le and it causes no new fails. OK for backport to
7 (and 6 if it's also needed there)?

Thanks,
   Aaron


On Fri, 2018-04-13 at 15:37 -0500, Aaron Sawdey wrote:
> Per the discussion on the 83660, I've come to a minimal patch to
> prevent this. Basically marking the vec_extract tree as having side
> effects later makes sure that it gets all the cleanup points it needs
> so that gimplify_cleanup_point_expr () is happy.  Also because
> vec_insert puts a MODIFY_EXPR in there, it has side effects and this
> problem will not occur.
> 
> Doing bootstrap/regtest on ppc64le with -mcpu=power7 since that is
> where this issue arises. OK for trunk if everything passes?
> 
> Thanks,
>    Aaron
> 
> 
> 2018-04-13  Aaron Sawdey  <acsawdey@linux.ibm.com>
> 
> 	PR target/83660
> 	* config/rs6000/rs6000-c.c
> (altivec_resolve_overloaded_builtin): Mark
> 	vec_extract expression as having side effects to make sure it
> gets
> 	a cleanup point.
> 
> 2018-04-13  Aaron Sawdey  <acsawdey@linux.ibm.com>
> 
> 	PR target/83660
> 	* gcc.target/powerpc/pr83660.C: New test.
>
Segher Boessenkool April 23, 2018, 4:42 p.m. | #3
On Mon, Apr 23, 2018 at 11:36:20AM -0500, Aaron Sawdey wrote:
> This also affects gcc 7 and is fixed by the same patch. I've tested the
> backport to 7 on ppc64le and it causes no new fails. OK for backport to
> 7 (and 6 if it's also needed there)?

Yes please (for both).  Thanks!


Segher


> On Fri, 2018-04-13 at 15:37 -0500, Aaron Sawdey wrote:
> > Per the discussion on the 83660, I've come to a minimal patch to
> > prevent this. Basically marking the vec_extract tree as having side
> > effects later makes sure that it gets all the cleanup points it needs
> > so that gimplify_cleanup_point_expr () is happy.  Also because
> > vec_insert puts a MODIFY_EXPR in there, it has side effects and this
> > problem will not occur.
> > 
> > Doing bootstrap/regtest on ppc64le with -mcpu=power7 since that is
> > where this issue arises. OK for trunk if everything passes?
> > 
> > Thanks,
> >    Aaron
> > 
> > 
> > 2018-04-13  Aaron Sawdey  <acsawdey@linux.ibm.com>
> > 
> > 	PR target/83660
> > 	* config/rs6000/rs6000-c.c
> > (altivec_resolve_overloaded_builtin): Mark
> > 	vec_extract expression as having side effects to make sure it
> > gets
> > 	a cleanup point.
> > 
> > 2018-04-13  Aaron Sawdey  <acsawdey@linux.ibm.com>
> > 
> > 	PR target/83660
> > 	* gcc.target/powerpc/pr83660.C: New test.

Patch

Index: config/rs6000/rs6000-c.c
===================================================================
--- config/rs6000/rs6000-c.c	(revision 259353)
+++ config/rs6000/rs6000-c.c	(working copy)
@@ -6705,6 +6705,15 @@ 
       stmt = build_binary_op (loc, PLUS_EXPR, stmt, arg2, 1);
       stmt = build_indirect_ref (loc, stmt, RO_NULL);
 
+      /* PR83660: We mark this as having side effects so that
+	 downstream in fold_build_cleanup_point_expr () it will get a
+	 CLEANUP_POINT_EXPR.  If it does not we can run into an ICE
+	 later in gimplify_cleanup_point_expr ().  Potentially this
+	 causes missed optimization because the actually is no side
+	 effect.  */
+      if (c_dialect_cxx ())
+	TREE_SIDE_EFFECTS (stmt) = 1;
+
       return stmt;
     }
 
Index: testsuite/gcc.target/powerpc/pr83660.C
===================================================================
--- testsuite/gcc.target/powerpc/pr83660.C	(nonexistent)
+++ testsuite/gcc.target/powerpc/pr83660.C	(working copy)
@@ -0,0 +1,14 @@ 
+/* PR target/83660 */
+/* { dg-do compile } */
+/* { dg-options "-mcpu=power7" } */
+
+#include <altivec.h>
+
+typedef __vector unsigned int  uvec32_t  __attribute__((__aligned__(16)));
+
+unsigned get_word(uvec32_t v)
+{
+    return ({const unsigned _B1 = 32;
+            vec_extract((uvec32_t)v, 2);});
+}
+