[PATCH] rs6000 PR83660 fix ICE with vec_extract

[PATCH] rs6000 PR83660 fix ICE with vec_extract

[Aaron Sawdey]

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

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.

-- 
Aaron Sawdey, Ph.D.  acsawdey@linux.vnet.ibm.com
050-2/C113  (507) 253-7520 home: 507/263-0782
IBM Linux Technology Center - PPC Toolchain

[-- Attachment #2: pr83660.patch --]
[-- Type: text/x-patch, Size: 1312 bytes --]

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);});
+}
+

Re: [PATCH] rs6000 PR83660 fix ICE with vec_extract

[Segher Boessenkool]

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

Re: [PATCH] rs6000 PR83660 fix ICE with vec_extract

[Aaron Sawdey]

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.
> 
-- 
Aaron Sawdey, Ph.D.  acsawdey@linux.vnet.ibm.com
050-2/C113  (507) 253-7520 home: 507/263-0782
IBM Linux Technology Center - PPC Toolchain

Re: [PATCH] rs6000 PR83660 fix ICE with vec_extract

[Segher Boessenkool]

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.