[PATCH, rs6000] Do not do gimple-folding of expressions that are missing their LHS

[PATCH, rs6000] Do not do gimple-folding of expressions that are missing their LHS

[Will Schmidt]

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

Re: [PATCH, rs6000] Do not do gimple-folding of expressions that are missing their LHS

[Bill Schmidt]

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

Re: [PATCH, rs6000] Do not do gimple-folding of expressions that are missing their LHS

[Segher Boessenkool]

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

Re: [PATCH, rs6000] Do not do gimple-folding of expressions that are missing their LHS

[Segher Boessenkool]

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

Re: [PATCH, rs6000] Do not do gimple-folding of expressions that are missing their LHS

[Richard Biener]

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