| Message ID | 1499877907.24125.400.camel@brimstone.rchland.ibm.com |
|---|---|
| State | New |
| Headers | show |
Although I said this was invalid code, it really isn't -- it's legal code. It's more of an ice-on-stupid-code situation. :) So probably you should remove the "invalid" language from the commentary. Sorry for misleading you. -- Bill Bill Schmidt, Ph.D. GCC for Linux on Power Linux on Power Toolchain IBM Linux Technology Center wschmidt@linux.vnet.ibm.com > On Jul 12, 2017, at 11:45 AM, Will Schmidt <will_schmidt@vnet.ibm.com> wrote: > > Hi, > > > Do not do the gimple-folding optimizations of expressions that are > missing their LHS. (Preventing an ICE on invalid code). > > This was noticed during debug of PR81317, but is not a fix for that PR. > This is based on a patch suggested by Segher. > > (This will need a revisit if/when we get as far as doing early gimple > folding for expressions without a lhs, but for now, this seems sufficient). > > This seems straightforward. regtest going on ppc64LE just in case. > OK for trunk? > > [gcc] > > 2017-07-12 Will Schmidt <will_schmidt@vnet.ibm.com> > > * config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Return > early if there is no lhs. > > [gcc/testsuite] > > 2017-07-12 Will Schmidt <will_schmidt@vnet.ibm.com> > > * gcc.target/powerpc/fold-vec-missing-lhs.c: New. > > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index 10c5521..e21b56f 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -16297,6 +16297,9 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) > = (enum rs6000_builtins) DECL_FUNCTION_CODE (fndecl); > tree arg0, arg1, lhs; > > + /* Generic solution to prevent gimple folding of code without a LHS. */ > + if (!gimple_call_lhs (stmt)) return false; > + > switch (fn_code) > { > /* Flavors of vec_add. We deliberately don't expand > diff --git a/gcc/testsuite/gcc.target/powerpc/fold-vec-missing-lhs.c b/gcc/testsuite/gcc.target/powerpc/fold-vec-missing-lhs.c > new file mode 100644 > index 0000000..d62f913 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-missing-lhs.c > @@ -0,0 +1,24 @@ > +/* This test is meant to verify that the gimple-folding does not > +occur when the LHS portion of an expression is missing. > +Though we would consider this invalid code, this should not generate an ICE. > +This was noticed during debug of PR81317. */ > + > +/* { dg-do compile } */ > +/* { dg-require-effective-target powerpc_altivec_ok } */ > +/* { dg-options "-maltivec" } */ > + > +#include <altivec.h> > + > +vector signed short > +test1_nolhs (vector bool short x, vector signed short y) > +{ > + vec_add (x, y); > + return vec_add (x, y); > +} > + > +vector signed short > +test2_nolhs (vector signed short x, vector bool short y) > +{ > + vec_add (x, y); > + return vec_add (x, y); > +} > >
On Wed, Jul 12, 2017 at 11:45:07AM -0500, Will Schmidt wrote: > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index 10c5521..e21b56f 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -16297,6 +16297,9 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) > = (enum rs6000_builtins) DECL_FUNCTION_CODE (fndecl); > tree arg0, arg1, lhs; > > + /* Generic solution to prevent gimple folding of code without a LHS. */ Only one space after /* please. > + if (!gimple_call_lhs (stmt)) return false; I know I typed that code, but "return" should be on a new line, indented. Sorry :-) > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-missing-lhs.c > @@ -0,0 +1,24 @@ > +/* This test is meant to verify that the gimple-folding does not > +occur when the LHS portion of an expression is missing. > +Though we would consider this invalid code, this should not generate an ICE. > +This was noticed during debug of PR81317. */ We usually indent comments, so three spaces at the start of each of the last three lines. Not that testcases really have to follow the GNU coding style. This comment is nicely informative in either case, thanks :-) The patch is fine with the typographical stuff fixed (and Bill's comment). Segher
On Wed, Jul 12, 2017 at 01:34:01PM -0500, Bill Schmidt wrote:
> Although I said this was invalid code, it really isn't -- it's legal code. It's more of an ice-on-stupid-code situation. :) So probably you should remove the "invalid" language from the commentary. Sorry for misleading you.
We could fold this to nothing (if there are no side effects), but it
would be better if we made stupid code slower instead of faster ;-)
Segher
On Wed, Jul 12, 2017 at 11:24 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Wed, Jul 12, 2017 at 01:34:01PM -0500, Bill Schmidt wrote: >> Although I said this was invalid code, it really isn't -- it's legal code. It's more of an ice-on-stupid-code situation. :) So probably you should remove the "invalid" language from the commentary. Sorry for misleading you. > > We could fold this to nothing (if there are no side effects), but it > would be better if we made stupid code slower instead of faster ;-) Well, optimization opportunities are not always obvious to the writer. Iff the builtins have no side-effects besides the return value the backend should mark them PURE or CONST and you wouldn't run into this situation. But yes, simply folding to GIMPLE_NOP is the appropriate thing when you want to paper over the above deficit in the folding routine. gsi_replace (si_p, gimple_build_nop (), false); note you'll eventually wreck virtual operands so before do unlink_stmt_vdef (gsi_stmt (gsi)); which may have it's own share of issues (folding may not look at SSA immediate uses...). So better fixup builtin attributes! Richard. > > Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 10c5521..e21b56f 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -16297,6 +16297,9 @@ rs6000_gimple_fold_builtin (gimple_stmt_iterator *gsi) = (enum rs6000_builtins) DECL_FUNCTION_CODE (fndecl); tree arg0, arg1, lhs; + /* Generic solution to prevent gimple folding of code without a LHS. */ + if (!gimple_call_lhs (stmt)) return false; + switch (fn_code) { /* Flavors of vec_add. We deliberately don't expand diff --git a/gcc/testsuite/gcc.target/powerpc/fold-vec-missing-lhs.c b/gcc/testsuite/gcc.target/powerpc/fold-vec-missing-lhs.c new file mode 100644 index 0000000..d62f913 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/fold-vec-missing-lhs.c @@ -0,0 +1,24 @@ +/* This test is meant to verify that the gimple-folding does not +occur when the LHS portion of an expression is missing. +Though we would consider this invalid code, this should not generate an ICE. +This was noticed during debug of PR81317. */ + +/* { dg-do compile } */ +/* { dg-require-effective-target powerpc_altivec_ok } */ +/* { dg-options "-maltivec" } */ + +#include <altivec.h> + +vector signed short +test1_nolhs (vector bool short x, vector signed short y) +{ + vec_add (x, y); + return vec_add (x, y); +} + +vector signed short +test2_nolhs (vector signed short x, vector bool short y) +{ + vec_add (x, y); + return vec_add (x, y); +}
Hi, Do not do the gimple-folding optimizations of expressions that are missing their LHS. (Preventing an ICE on invalid code). This was noticed during debug of PR81317, but is not a fix for that PR. This is based on a patch suggested by Segher. (This will need a revisit if/when we get as far as doing early gimple folding for expressions without a lhs, but for now, this seems sufficient). This seems straightforward. regtest going on ppc64LE just in case. OK for trunk? [gcc] 2017-07-12 Will Schmidt <will_schmidt@vnet.ibm.com> * config/rs6000/rs6000.c (rs6000_gimple_fold_builtin): Return early if there is no lhs. [gcc/testsuite] 2017-07-12 Will Schmidt <will_schmidt@vnet.ibm.com> * gcc.target/powerpc/fold-vec-missing-lhs.c: New.