forwprop: simplify_vector_constructor follow-up fix [PR95713]

Message ID 20200618093242.GO8462@tucnak
State New
Headers show
Series
  • forwprop: simplify_vector_constructor follow-up fix [PR95713]
Related show

Commit Message

Christophe Lyon via Gcc-patches June 18, 2020, 9:32 a.m.
Hi!

As the following testcase shows, the exception for the aarch64
vec_pack_trunc_di is not sufficient on x86, the halfvectype
"vectors" have SImode but the x86 vec_pack_trunc_si meant for
the bool bitmasks combines 2x SImode into DImode, while in the
testcase the halfvectype is 1x SImode "vector" with SImode and
result is 2x HImode "vector" with SImode.

The patch also verifies the result mode.  The other option is
to throw away the aarch64 exception.  And yet another option
would be to use different optabs for vector bools with integral modes.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/10.2?

2020-06-18  Jakub Jelinek  <jakub@redhat.com>

	PR target/95713
	* tree-ssa-forwprop.c: Include insn-config.h and recog.h.
	(simplify_vector_constructor): For integral TYPE_MODE of
	non-VECTOR_BOOLEAN_P vector same as element mode, verify also
	that lhs mode matches the output operand mode of the expander.

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


	Jakub

Comments

Richard Biener June 18, 2020, 9:55 a.m. | #1
On Thu, 18 Jun 2020, Jakub Jelinek wrote:

> Hi!

> 

> As the following testcase shows, the exception for the aarch64

> vec_pack_trunc_di is not sufficient on x86, the halfvectype

> "vectors" have SImode but the x86 vec_pack_trunc_si meant for

> the bool bitmasks combines 2x SImode into DImode, while in the

> testcase the halfvectype is 1x SImode "vector" with SImode and

> result is 2x HImode "vector" with SImode.

> 

> The patch also verifies the result mode.  The other option is

> to throw away the aarch64 exception.  And yet another option

> would be to use different optabs for vector bools with integral modes.

> 

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/10.2?


OK.

Given we now have expanders for the actual conversion operation on
x86_64 as well we might consider removing all this VEC_[UN]PACK[_FLOAT]_..
stuff again.  Though I guess the VEC_[UN]PACK_FLOAT variants are
still not covered by vector mode [us]float_optab and
vector typed FLOAT_EXPR?

Thanks,
Richard.

> 2020-06-18  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR target/95713

> 	* tree-ssa-forwprop.c: Include insn-config.h and recog.h.

> 	(simplify_vector_constructor): For integral TYPE_MODE of

> 	non-VECTOR_BOOLEAN_P vector same as element mode, verify also

> 	that lhs mode matches the output operand mode of the expander.

> 

> 	* gcc.dg/pr95713.c: New test.

> 

> --- gcc/tree-ssa-forwprop.c.jj	2020-06-08 11:05:04.196939262 +0200

> +++ gcc/tree-ssa-forwprop.c	2020-06-17 11:18:10.816155772 +0200

> @@ -51,6 +51,8 @@ along with GCC; see the file COPYING3.

>  #include "internal-fn.h"

>  #include "cgraph.h"

>  #include "tree-ssa.h"

> +#include "insn-config.h"

> +#include "recog.h"

>  

>  /* This pass propagates the RHS of assignment statements into use

>     sites of the LHS of the assignment.  It's basically a specialized

> @@ -2391,7 +2393,8 @@ simplify_vector_constructor (gimple_stmt

>  	  /* Only few targets implement direct conversion patterns so try

>  	     some simple special cases via VEC_[UN]PACK[_FLOAT]_LO_EXPR.  */

>  	  optab optab;

> -	  tree halfvectype, dblvectype;

> +	  enum insn_code icode;

> +	  tree halfvectype, dblvectype, lhs;

>  	  if (CONVERT_EXPR_CODE_P (conv_code)

>  	      && (2 * TYPE_PRECISION (TREE_TYPE (TREE_TYPE (orig[0])))

>  		  == TYPE_PRECISION (TREE_TYPE (type)))

> @@ -2446,18 +2449,21 @@ simplify_vector_constructor (gimple_stmt

>  		   && (halfvectype

>  		         = build_vector_type (TREE_TYPE (TREE_TYPE (orig[0])),

>  					      nelts / 2))

> +		   && (optab = optab_for_tree_code (VEC_PACK_TRUNC_EXPR,

> +						    halfvectype,

> +						    optab_default))

> +		   && ((icode = optab_handler (optab, TYPE_MODE (halfvectype)))

> +		       != CODE_FOR_nothing)

>  		   /* Only use it for vector modes or for vector booleans

>  		      represented as scalar bitmasks, or allow halfvectype

>  		      be the element mode.  See PR95528.  */

>  		   && (VECTOR_MODE_P (TYPE_MODE (halfvectype))

>  		       || VECTOR_BOOLEAN_TYPE_P (halfvectype)

>  		       || (TYPE_MODE (halfvectype)

> -			   == TYPE_MODE (TREE_TYPE (halfvectype))))

> -		   && (optab = optab_for_tree_code (VEC_PACK_TRUNC_EXPR,

> -						    halfvectype,

> -						    optab_default))

> -		   && (optab_handler (optab, TYPE_MODE (halfvectype))

> -		       != CODE_FOR_nothing))

> +			   == TYPE_MODE (TREE_TYPE (halfvectype))

> +			   && (lhs == gimple_assign_lhs (stmt))

> +			   && (insn_data[icode].operand[0].mode

> +			       == TYPE_MODE (TREE_TYPE (lhs))))))

>  	    {

>  	      gimple_seq stmts = NULL;

>  	      tree low = gimple_build (&stmts, BIT_FIELD_REF, halfvectype,

> --- gcc/testsuite/gcc.dg/pr95713.c.jj	2020-06-17 11:20:12.288381166 +0200

> +++ gcc/testsuite/gcc.dg/pr95713.c	2020-06-17 11:19:56.057618283 +0200

> @@ -0,0 +1,15 @@

> +/* PR target/95713 */

> +/* { dg-do compile } */

> +/* { dg-options "-O2 -Wno-psabi -w" } */

> +/* { dg-additional-options "-mavx512bw" { target i?86-*-* x86_64-*-* } } */

> +

> +typedef int v2si __attribute__((vector_size (8)));

> +typedef short int v2hi __attribute__((vector_size (4)));

> +void foo (v2hi);

> +

> +void

> +bar (v2si x)

> +{

> +  v2hi a = (v2hi) { (short) x[0], (short) x[1] };

> +  foo (4 > a);

> +}

> 

> 	Jakub

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Richard Sandiford June 18, 2020, 9:59 a.m. | #2
Richard Biener <rguenther@suse.de> writes:
> On Thu, 18 Jun 2020, Jakub Jelinek wrote:

>

>> Hi!

>> 

>> As the following testcase shows, the exception for the aarch64

>> vec_pack_trunc_di is not sufficient on x86, the halfvectype

>> "vectors" have SImode but the x86 vec_pack_trunc_si meant for

>> the bool bitmasks combines 2x SImode into DImode, while in the

>> testcase the halfvectype is 1x SImode "vector" with SImode and

>> result is 2x HImode "vector" with SImode.

>> 

>> The patch also verifies the result mode.  The other option is

>> to throw away the aarch64 exception.  And yet another option

>> would be to use different optabs for vector bools with integral modes.

>> 

>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/10.2?

>

> OK.


FWIW, since the aarch64 case was only found by inspection and might
not be useful, personally I'd prefer to drop that case after all.

Thanks,
Richard

Patch

--- gcc/tree-ssa-forwprop.c.jj	2020-06-08 11:05:04.196939262 +0200
+++ gcc/tree-ssa-forwprop.c	2020-06-17 11:18:10.816155772 +0200
@@ -51,6 +51,8 @@  along with GCC; see the file COPYING3.
 #include "internal-fn.h"
 #include "cgraph.h"
 #include "tree-ssa.h"
+#include "insn-config.h"
+#include "recog.h"
 
 /* This pass propagates the RHS of assignment statements into use
    sites of the LHS of the assignment.  It's basically a specialized
@@ -2391,7 +2393,8 @@  simplify_vector_constructor (gimple_stmt
 	  /* Only few targets implement direct conversion patterns so try
 	     some simple special cases via VEC_[UN]PACK[_FLOAT]_LO_EXPR.  */
 	  optab optab;
-	  tree halfvectype, dblvectype;
+	  enum insn_code icode;
+	  tree halfvectype, dblvectype, lhs;
 	  if (CONVERT_EXPR_CODE_P (conv_code)
 	      && (2 * TYPE_PRECISION (TREE_TYPE (TREE_TYPE (orig[0])))
 		  == TYPE_PRECISION (TREE_TYPE (type)))
@@ -2446,18 +2449,21 @@  simplify_vector_constructor (gimple_stmt
 		   && (halfvectype
 		         = build_vector_type (TREE_TYPE (TREE_TYPE (orig[0])),
 					      nelts / 2))
+		   && (optab = optab_for_tree_code (VEC_PACK_TRUNC_EXPR,
+						    halfvectype,
+						    optab_default))
+		   && ((icode = optab_handler (optab, TYPE_MODE (halfvectype)))
+		       != CODE_FOR_nothing)
 		   /* Only use it for vector modes or for vector booleans
 		      represented as scalar bitmasks, or allow halfvectype
 		      be the element mode.  See PR95528.  */
 		   && (VECTOR_MODE_P (TYPE_MODE (halfvectype))
 		       || VECTOR_BOOLEAN_TYPE_P (halfvectype)
 		       || (TYPE_MODE (halfvectype)
-			   == TYPE_MODE (TREE_TYPE (halfvectype))))
-		   && (optab = optab_for_tree_code (VEC_PACK_TRUNC_EXPR,
-						    halfvectype,
-						    optab_default))
-		   && (optab_handler (optab, TYPE_MODE (halfvectype))
-		       != CODE_FOR_nothing))
+			   == TYPE_MODE (TREE_TYPE (halfvectype))
+			   && (lhs == gimple_assign_lhs (stmt))
+			   && (insn_data[icode].operand[0].mode
+			       == TYPE_MODE (TREE_TYPE (lhs))))))
 	    {
 	      gimple_seq stmts = NULL;
 	      tree low = gimple_build (&stmts, BIT_FIELD_REF, halfvectype,
--- gcc/testsuite/gcc.dg/pr95713.c.jj	2020-06-17 11:20:12.288381166 +0200
+++ gcc/testsuite/gcc.dg/pr95713.c	2020-06-17 11:19:56.057618283 +0200
@@ -0,0 +1,15 @@ 
+/* PR target/95713 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wno-psabi -w" } */
+/* { dg-additional-options "-mavx512bw" { target i?86-*-* x86_64-*-* } } */
+
+typedef int v2si __attribute__((vector_size (8)));
+typedef short int v2hi __attribute__((vector_size (4)));
+void foo (v2hi);
+
+void
+bar (v2si x)
+{
+  v2hi a = (v2hi) { (short) x[0], (short) x[1] };
+  foo (4 > a);
+}