Guard binary/ternary match.pd patterns to IFN_COND_* with IFN_COND_* availability (PR tree-optimization/89570)

Hi!

As the following testcase shows, these match.pd patterns create temporary
GIMPLE stmts even when they aren't going to result in anything useful
(all targets except aarch64 right now), besides compile time memory
this is bad with -fno-tree-dce because those stmts might not be even valid
for the target and we might ICE during expansion.

Fixed by guarding them with a vectorized_internal_fn_supported_p test.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Note, I have no idea how to test this on aarch64, Richard S., can you please
do that?  Thanks.

2019-03-04  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/89570
	* match.pd (vec_cond into cond_op simplification): Guard with
	vectorized_internal_fn_supported_p test and #if GIMPLE.

	* gcc.dg/pr89570.c: New test.


	Jakub

On Mon, 4 Mar 2019, Jakub Jelinek wrote:

> Hi!
> 
> As the following testcase shows, these match.pd patterns create temporary
> GIMPLE stmts even when they aren't going to result in anything useful
> (all targets except aarch64 right now), besides compile time memory
> this is bad with -fno-tree-dce because those stmts might not be even valid
> for the target and we might ICE during expansion.
> 
> Fixed by guarding them with a vectorized_internal_fn_supported_p test.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Richard.

> Note, I have no idea how to test this on aarch64, Richard S., can you please
> do that?  Thanks.
> 
> 2019-03-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/89570
> 	* match.pd (vec_cond into cond_op simplification): Guard with
> 	vectorized_internal_fn_supported_p test and #if GIMPLE.
> 
> 	* gcc.dg/pr89570.c: New test.
> 
> --- gcc/match.pd.jj	2019-01-16 09:35:08.421259263 +0100
> +++ gcc/match.pd	2019-03-04 13:00:02.884284658 +0100
> @@ -5177,17 +5177,24 @@ (define_operator_list COND_TERNARY
>     if the target can do it in one go.  This makes the operation conditional
>     on c, so could drop potentially-trapping arithmetic, but that's a valid
>     simplification if the result of the operation isn't needed.  */
> +#if GIMPLE
>  (for uncond_op (UNCOND_BINARY)
>       cond_op (COND_BINARY)
>   (simplify
>    (vec_cond @0 (view_convert? (uncond_op@4 @1 @2)) @3)
> -  (with { tree op_type = TREE_TYPE (@4); }
> -   (if (element_precision (type) == element_precision (op_type))
> +  (with { tree op_type = TREE_TYPE (@4); 
> +	  internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
> +   (if (cond_fn != IFN_LAST
> +	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
> +	&& element_precision (type) == element_precision (op_type))
>      (view_convert (cond_op @0 @1 @2 (view_convert:op_type @3))))))
>   (simplify
>    (vec_cond @0 @1 (view_convert? (uncond_op@4 @2 @3)))
> -  (with { tree op_type = TREE_TYPE (@4); }
> -   (if (element_precision (type) == element_precision (op_type))
> +  (with { tree op_type = TREE_TYPE (@4);
> +	  internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
> +   (if (cond_fn != IFN_LAST
> +	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
> +	&& element_precision (type) == element_precision (op_type))
>      (view_convert (cond_op (bit_not @0) @2 @3 (view_convert:op_type @1)))))))
>  
>  /* Same for ternary operations.  */
> @@ -5195,15 +5202,24 @@ (define_operator_list COND_TERNARY
>       cond_op (COND_TERNARY)
>   (simplify
>    (vec_cond @0 (view_convert? (uncond_op@5 @1 @2 @3)) @4)
> -  (with { tree op_type = TREE_TYPE (@5); }
> -   (if (element_precision (type) == element_precision (op_type))
> +  (with { tree op_type = TREE_TYPE (@5);
> +	  internal_fn cond_fn
> +	    = get_conditional_internal_fn (as_internal_fn (uncond_op)); }
> +   (if (cond_fn != IFN_LAST
> +	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
> +	&& element_precision (type) == element_precision (op_type))
>      (view_convert (cond_op @0 @1 @2 @3 (view_convert:op_type @4))))))
>   (simplify
>    (vec_cond @0 @1 (view_convert? (uncond_op@5 @2 @3 @4)))
> -  (with { tree op_type = TREE_TYPE (@5); }
> -   (if (element_precision (type) == element_precision (op_type))
> +  (with { tree op_type = TREE_TYPE (@5);
> +	  internal_fn cond_fn
> +	    = get_conditional_internal_fn (as_internal_fn (uncond_op)); }
> +   (if (cond_fn != IFN_LAST
> +	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
> +	&& element_precision (type) == element_precision (op_type))
>      (view_convert (cond_op (bit_not @0) @2 @3 @4
>  		  (view_convert:op_type @1)))))))
> +#endif
>  
>  /* Detect cases in which a VEC_COND_EXPR effectively replaces the
>     "else" value of an IFN_COND_*.  */
> --- gcc/testsuite/gcc.dg/pr89570.c.jj	2019-03-04 13:04:00.459544926 +0100
> +++ gcc/testsuite/gcc.dg/pr89570.c	2019-03-04 13:03:44.157801534 +0100
> @@ -0,0 +1,15 @@
> +/* PR tree-optimization/89570 */
> +/* { dg-do compile } */
> +/* { dg-options "-O1 -ftree-vectorize -fno-trapping-math -fno-tree-dce -fno-tree-dominator-opts" } */
> +/* { dg-additional-options "-mvsx" { target powerpc_vsx_ok } } */
> +
> +void
> +foo (double *x, double *y, double *z)
> +{
> +  int i;
> +  for (i = 0; i < 7; i += 2)
> +    {
> +      x[i] = y[i] ? z[i] / 2.0 : z[i];
> +      x[i + 1] = y[i + 1] ? z[i + 1] / 2.0 : z[i + 1];
> +    }
> +}
> 
> 	Jakub
> 
>

Jakub Jelinek <jakub@redhat.com> writes:
> Hi!
>
> As the following testcase shows, these match.pd patterns create temporary
> GIMPLE stmts even when they aren't going to result in anything useful
> (all targets except aarch64 right now), besides compile time memory
> this is bad with -fno-tree-dce because those stmts might not be even valid
> for the target and we might ICE during expansion.
>
> Fixed by guarding them with a vectorized_internal_fn_supported_p test.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Note, I have no idea how to test this on aarch64, Richard S., can you please
> do that?  Thanks.

Sorry, commented on the bug before seeing this patch.

I don't think this is the way to go though.  If we want match.pd
rules to have early checks for whether an ifn is supported, I think we
should get genmatch to do that itself rather than have to remember
to do it manually for each match.pd condition.

In this case, isn't the underying problem that we only support some
vector conditions in VEC_COND_EXPRs and not as stand-alone comparisons?
Shouldn't we address that directly and then treat the early checks as
a separate compile-time optimisation?

As far as the patch itself goes, I don't understand why you have:

      internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }

when the cond_op iterator already gives the conditional internal function.

Actually... I see this went while writing the email, but it still seems
like the wrong approach to me.

Thanks,
Richard

On Tue, Mar 05, 2019 at 08:53:00AM +0000, Richard Sandiford wrote:
> Sorry, commented on the bug before seeing this patch.

Sorry, I've committed before seeing your comment in the PR
(and responded there after seeing it).

> I don't think this is the way to go though.  If we want match.pd
> rules to have early checks for whether an ifn is supported, I think we
> should get genmatch to do that itself rather than have to remember
> to do it manually for each match.pd condition.

Why?  Most of the patterns don't introduce IFN_COND_* out of other code,
just simplify existing IFN_COND_*.  And those that adjust existing ones
don't need these extra checks first.

> In this case, isn't the underying problem that we only support some
> vector conditions in VEC_COND_EXPRs and not as stand-alone comparisons?

That is not a bug, I believe VEC_COND_EXPRs have been supported far earlier
than those vec_cmp* optabs have been and VEC_COND_EXPRs is the only thing
you want to use on targets which don't really have any mask registers for
vectors, where the result of comparison is really vectorized x == y ? -1 : 0.
vec_cmp* have been introduced for AVX512F I believe and are for the case
when you store the result of the comparison in some bitset (mask), usually
the mask has some other type than the vector it is comparing etc.
(VECTOR_BOOLEAN_P at the GIMPLE type usually).

> Shouldn't we address that directly and then treat the early checks as
> a separate compile-time optimisation?
> 
> As far as the patch itself goes, I don't understand why you have:
> 
>       internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
> 
> when the cond_op iterator already gives the conditional internal function.

I guess you're right and that could simplify the changes.
That would be (untested except for make cc1):

2019-03-05  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/89570
	* match.pd (vec_cond into cond_op simplification): Don't use
	get_conditional_internal_fn, use as_internal_fn (cond_op).

--- gcc/match.pd.jj	2019-03-05 09:43:46.555727525 +0100
+++ gcc/match.pd	2019-03-05 10:05:20.089630138 +0100
@@ -5182,18 +5182,14 @@ (define_operator_list COND_TERNARY
      cond_op (COND_BINARY)
  (simplify
   (vec_cond @0 (view_convert? (uncond_op@4 @1 @2)) @3)
-  (with { tree op_type = TREE_TYPE (@4); 
-	  internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
-   (if (cond_fn != IFN_LAST
-	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
+  (with { tree op_type = TREE_TYPE (@4); }
+   (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type)
 	&& element_precision (type) == element_precision (op_type))
     (view_convert (cond_op @0 @1 @2 (view_convert:op_type @3))))))
  (simplify
   (vec_cond @0 @1 (view_convert? (uncond_op@4 @2 @3)))
-  (with { tree op_type = TREE_TYPE (@4);
-	  internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
-   (if (cond_fn != IFN_LAST
-	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
+  (with { tree op_type = TREE_TYPE (@4); }
+   (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type)
 	&& element_precision (type) == element_precision (op_type))
     (view_convert (cond_op (bit_not @0) @2 @3 (view_convert:op_type @1)))))))
 
@@ -5202,20 +5198,14 @@ (define_operator_list COND_TERNARY
      cond_op (COND_TERNARY)
  (simplify
   (vec_cond @0 (view_convert? (uncond_op@5 @1 @2 @3)) @4)
-  (with { tree op_type = TREE_TYPE (@5);
-	  internal_fn cond_fn
-	    = get_conditional_internal_fn (as_internal_fn (uncond_op)); }
-   (if (cond_fn != IFN_LAST
-	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
+  (with { tree op_type = TREE_TYPE (@5); }
+   (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type)
 	&& element_precision (type) == element_precision (op_type))
     (view_convert (cond_op @0 @1 @2 @3 (view_convert:op_type @4))))))
  (simplify
   (vec_cond @0 @1 (view_convert? (uncond_op@5 @2 @3 @4)))
-  (with { tree op_type = TREE_TYPE (@5);
-	  internal_fn cond_fn
-	    = get_conditional_internal_fn (as_internal_fn (uncond_op)); }
-   (if (cond_fn != IFN_LAST
-	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
+  (with { tree op_type = TREE_TYPE (@5); }
+   (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type)
 	&& element_precision (type) == element_precision (op_type))
     (view_convert (cond_op (bit_not @0) @2 @3 @4
 		  (view_convert:op_type @1)))))))


	Jakub

On Tue, 5 Mar 2019, Richard Sandiford wrote:

> Jakub Jelinek <jakub@redhat.com> writes:
> > Hi!
> >
> > As the following testcase shows, these match.pd patterns create temporary
> > GIMPLE stmts even when they aren't going to result in anything useful
> > (all targets except aarch64 right now), besides compile time memory
> > this is bad with -fno-tree-dce because those stmts might not be even valid
> > for the target and we might ICE during expansion.
> >
> > Fixed by guarding them with a vectorized_internal_fn_supported_p test.
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > Note, I have no idea how to test this on aarch64, Richard S., can you please
> > do that?  Thanks.
> 
> Sorry, commented on the bug before seeing this patch.
> 
> I don't think this is the way to go though.  If we want match.pd
> rules to have early checks for whether an ifn is supported, I think we
> should get genmatch to do that itself rather than have to remember
> to do it manually for each match.pd condition.

But vector IFNs/operations are special (unfortunately).  Look how
we need to check for valid constant permutations for example.

> In this case, isn't the underying problem that we only support some
> vector conditions in VEC_COND_EXPRs and not as stand-alone comparisons?
> Shouldn't we address that directly and then treat the early checks as
> a separate compile-time optimisation?
> 
> As far as the patch itself goes, I don't understand why you have:
> 
>       internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
> 
> when the cond_op iterator already gives the conditional internal function.
> 
> Actually... I see this went while writing the email, but it still seems
> like the wrong approach to me.

Richard.

Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Mar 05, 2019 at 08:53:00AM +0000, Richard Sandiford wrote:
>> Sorry, commented on the bug before seeing this patch.
>
> Sorry, I've committed before seeing your comment in the PR
> (and responded there after seeing it).
>
>> I don't think this is the way to go though.  If we want match.pd
>> rules to have early checks for whether an ifn is supported, I think we
>> should get genmatch to do that itself rather than have to remember
>> to do it manually for each match.pd condition.
>
> Why?  Most of the patterns don't introduce IFN_COND_* out of other code,
> just simplify existing IFN_COND_*.  And those that adjust existing ones
> don't need these extra checks first.
>
>> In this case, isn't the underying problem that we only support some
>> vector conditions in VEC_COND_EXPRs and not as stand-alone comparisons?
>
> That is not a bug, I believe VEC_COND_EXPRs have been supported far earlier
> than those vec_cmp* optabs have been and VEC_COND_EXPRs is the only thing
> you want to use on targets which don't really have any mask registers for
> vectors, where the result of comparison is really vectorized x == y ? -1 : 0.
> vec_cmp* have been introduced for AVX512F I believe and are for the case
> when you store the result of the comparison in some bitset (mask), usually
> the mask has some other type than the vector it is comparing etc.
> (VECTOR_BOOLEAN_P at the GIMPLE type usually).
>
>> Shouldn't we address that directly and then treat the early checks as
>> a separate compile-time optimisation?
>> 
>> As far as the patch itself goes, I don't understand why you have:
>> 
>>       internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
>> 
>> when the cond_op iterator already gives the conditional internal function.
>
> I guess you're right and that could simplify the changes.
> That would be (untested except for make cc1):

LGTM, thanks.  Given the discussion, I think it would also be worth having
a comment explaining why we're doing this, something like:

  /* Avoid speculatively generating a stand-alone vector comparison
     on targets that might not support them.  Any target implementing
     conditional internal functions must support the same comparisons
     inside and outside a VEC_COND_EXPR.  */

Richard

PS. Sorry for not commenting yesterday.  I've now switched my
    gcc.gnu.org email to my work address, so hopefully I'll be
    a bit more responsive to bugzilla stuff in future.

On Tue, Mar 05, 2019 at 09:26:19AM +0000, Richard Sandiford wrote:
> LGTM, thanks.  Given the discussion, I think it would also be worth having
> a comment explaining why we're doing this, something like:
> 
>   /* Avoid speculatively generating a stand-alone vector comparison
>      on targets that might not support them.  Any target implementing
>      conditional internal functions must support the same comparisons
>      inside and outside a VEC_COND_EXPR.  */

Ok, here is the patch updated with your comment.

Can you please test it on aarch64 SVE?  I'll test it on x86_64/i686 later
today (maybe powerpc64{,le} too).

2019-03-05  Jakub Jelinek  <jakub@redhat.com>
	    Richard Sandiford  <richard.sandiford@arm.com>

	PR tree-optimization/89570
	* match.pd (vec_cond into cond_op simplification): Don't use
	get_conditional_internal_fn, use as_internal_fn (cond_op).

--- gcc/match.pd.jj	2019-03-05 09:43:46.555727525 +0100
+++ gcc/match.pd	2019-03-05 10:05:20.089630138 +0100
@@ -5176,24 +5176,26 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 
    if the target can do it in one go.  This makes the operation conditional
    on c, so could drop potentially-trapping arithmetic, but that's a valid
-   simplification if the result of the operation isn't needed.  */
+   simplification if the result of the operation isn't needed.
+
+   Avoid speculatively generating a stand-alone vector comparison                                                                                
+   on targets that might not support them.  Any target implementing                                                                              
+   conditional internal functions must support the same comparisons                                                                              
+   inside and outside a VEC_COND_EXPR.  */                                                                                                       
+
 #if GIMPLE
 (for uncond_op (UNCOND_BINARY)
      cond_op (COND_BINARY)
  (simplify
   (vec_cond @0 (view_convert? (uncond_op@4 @1 @2)) @3)
-  (with { tree op_type = TREE_TYPE (@4); 
-	  internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
-   (if (cond_fn != IFN_LAST
-	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
+  (with { tree op_type = TREE_TYPE (@4); }
+   (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type)
 	&& element_precision (type) == element_precision (op_type))
     (view_convert (cond_op @0 @1 @2 (view_convert:op_type @3))))))
  (simplify
   (vec_cond @0 @1 (view_convert? (uncond_op@4 @2 @3)))
-  (with { tree op_type = TREE_TYPE (@4);
-	  internal_fn cond_fn = get_conditional_internal_fn (uncond_op); }
-   (if (cond_fn != IFN_LAST
-	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
+  (with { tree op_type = TREE_TYPE (@4); }
+   (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type)
 	&& element_precision (type) == element_precision (op_type))
     (view_convert (cond_op (bit_not @0) @2 @3 (view_convert:op_type @1)))))))
 
@@ -5202,20 +5204,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
      cond_op (COND_TERNARY)
  (simplify
   (vec_cond @0 (view_convert? (uncond_op@5 @1 @2 @3)) @4)
-  (with { tree op_type = TREE_TYPE (@5);
-	  internal_fn cond_fn
-	    = get_conditional_internal_fn (as_internal_fn (uncond_op)); }
-   (if (cond_fn != IFN_LAST
-	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
+  (with { tree op_type = TREE_TYPE (@5); }
+   (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type)
 	&& element_precision (type) == element_precision (op_type))
     (view_convert (cond_op @0 @1 @2 @3 (view_convert:op_type @4))))))
  (simplify
   (vec_cond @0 @1 (view_convert? (uncond_op@5 @2 @3 @4)))
-  (with { tree op_type = TREE_TYPE (@5);
-	  internal_fn cond_fn
-	    = get_conditional_internal_fn (as_internal_fn (uncond_op)); }
-   (if (cond_fn != IFN_LAST
-	&& vectorized_internal_fn_supported_p (cond_fn, op_type)
+  (with { tree op_type = TREE_TYPE (@5); }
+   (if (vectorized_internal_fn_supported_p (as_internal_fn (cond_op), op_type)
 	&& element_precision (type) == element_precision (op_type))
     (view_convert (cond_op (bit_not @0) @2 @3 @4
 		  (view_convert:op_type @1)))))))


	Jakub

Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Mar 05, 2019 at 09:26:19AM +0000, Richard Sandiford wrote:
>> LGTM, thanks.  Given the discussion, I think it would also be worth having
>> a comment explaining why we're doing this, something like:
>> 
>>   /* Avoid speculatively generating a stand-alone vector comparison
>>      on targets that might not support them.  Any target implementing
>>      conditional internal functions must support the same comparisons
>>      inside and outside a VEC_COND_EXPR.  */
>
> Ok, here is the patch updated with your comment.
>
> Can you please test it on aarch64 SVE?  I'll test it on x86_64/i686 later
> today (maybe powerpc64{,le} too).

OK, testing passed on SVE.

Thanks,
Richard