| Message ID | 1523651828.6245.26.camel@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000 PR83660 fix ICE with vec_extract | expand |
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
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. >
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.
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);}); +} +