diff mbox series

Fix vect rotate pattern recog (PR target/92723)

Message ID 20191210205405.GG10088@tucnak
State New
Headers show
Series Fix vect rotate pattern recog (PR target/92723) | expand

Commit Message

Jakub Jelinek Dec. 10, 2019, 8:54 p.m. UTC 
Hi!

Unlike x86, where the last operand of vector by scalar shift is DImode for
V[248]DImode shifts, on aarch64 they seem to be SImode.
vect_recog_rotate_pattern when the rotate amount is not constant casts the
amount to the element type of the vector, so for V[248]DImode vectors to
DImode, but then we ICE during expand_shift_1 because such argument doesn't
satisfy the predicate and can't be widened to it.

The following patch fixes it by special casing vector by scalar shifts
when adding patterns for rotates, in that case we punt if the operand isn't
INTEGER_CST or external_def, and the patch just keeps using the type of the
amount operand the rotate had for the shifts too.

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

2019-12-10  Jakub Jelinek  <jakub@redhat.com>

	PR target/92723
	* tree-vect-patterns.c (vect_recog_rotate_pattern): If vector x vector
	shifts aren't supported and rotate amount is SSA_NAME, use its type
	rather than first operand's type for the shift amounts.

	* gcc.dg/vect/pr92723.c: New test.


	Jakub

Comments

Richard Sandiford Dec. 11, 2019, 4:52 p.m. UTC | #1 
Jakub Jelinek <jakub@redhat.com> writes:
> Hi!
>
> Unlike x86, where the last operand of vector by scalar shift is DImode for
> V[248]DImode shifts, on aarch64 they seem to be SImode.
> vect_recog_rotate_pattern when the rotate amount is not constant casts the
> amount to the element type of the vector, so for V[248]DImode vectors to
> DImode, but then we ICE during expand_shift_1 because such argument doesn't
> satisfy the predicate and can't be widened to it.
>
> The following patch fixes it by special casing vector by scalar shifts
> when adding patterns for rotates, in that case we punt if the operand isn't
> INTEGER_CST or external_def, and the patch just keeps using the type of the
> amount operand the rotate had for the shifts too.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2019-12-10  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/92723
> 	* tree-vect-patterns.c (vect_recog_rotate_pattern): If vector x vector
> 	shifts aren't supported and rotate amount is SSA_NAME, use its type
> 	rather than first operand's type for the shift amounts.
>
> 	* gcc.dg/vect/pr92723.c: New test.
>
> --- gcc/tree-vect-patterns.c.jj	2019-12-09 11:12:29.983288823 +0100
> +++ gcc/tree-vect-patterns.c	2019-12-10 16:30:59.922177911 +0100
> @@ -2242,6 +2242,7 @@ vect_recog_rotate_pattern (stmt_vec_info
>    optab optab1, optab2;
>    edge ext_def = NULL;
>    bool bswap16_p = false;
> +  bool scalar_shift_p = false;
>  
>    if (is_gimple_assign (last_stmt))
>      {
> @@ -2420,6 +2421,7 @@ vect_recog_rotate_pattern (stmt_vec_info
>  	  || !optab2
>  	  || optab_handler (optab2, TYPE_MODE (vectype)) == CODE_FOR_nothing)
>  	return NULL;
> +      scalar_shift_p = true;
>      }
>  
>    *type_out = vectype;
> @@ -2439,7 +2441,8 @@ vect_recog_rotate_pattern (stmt_vec_info
>    def = NULL_TREE;
>    scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
>    if (TREE_CODE (oprnd1) == INTEGER_CST
> -      || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)
> +      || TYPE_MODE (TREE_TYPE (oprnd1)) == mode
> +      || scalar_shift_p)
>      def = oprnd1;
>    else if (def_stmt && gimple_assign_cast_p (def_stmt))
>      {

WDYT about instead having:

  if (dt != vect_internal_def || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)

and removing the ext_def stuff?  I'd have expected keeping the original
operand to always be best for vect_external_def, and it avoids changing
the function body during what's supposed to be just an analysis phase.

Thanks,
Richard

> --- gcc/testsuite/gcc.dg/vect/pr92723.c.jj	2019-12-10 16:37:09.924375698 +0100
> +++ gcc/testsuite/gcc.dg/vect/pr92723.c	2019-12-10 16:37:21.823189103 +0100
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +
> +void
> +foo (unsigned long long *x, unsigned long long *y, int z)
> +{
> +  int i;
> +  for (i = 0; i < 1024; i++)
> +    x[i] = (y[i] >> z) | (y[i] << (-z & (__SIZEOF_LONG_LONG__ * __CHAR_BIT__ - 1)));
> +}
>
> 	Jakub
Jakub Jelinek Dec. 11, 2019, 5:25 p.m. UTC | #2 
On Wed, Dec 11, 2019 at 04:52:30PM +0000, Richard Sandiford wrote:
> WDYT about instead having:
> 
>   if (dt != vect_internal_def || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)
> 
> and removing the ext_def stuff?  I'd have expected keeping the original
> operand to always be best for vect_external_def, and it avoids changing
> the function body during what's supposed to be just an analysis phase.

The ext_def stuff is needed in any case, for the -amount & mask expression.
If all you mean is following, then I think it should work and can
bootstrap/regtest it tonight (though just on x86_64/i686):

2019-12-11  Jakub Jelinek  <jakub@redhat.com>

	PR target/92723
	* tree-vect-patterns.c (vect_recog_rotate_pattern): If dt is not
	vect_internal_def, use oprnd1 as is, without trying to cast it.
	Formatting fix.

	* gcc.dg/vect/pr92723.c: New test.

--- gcc/tree-vect-patterns.c.jj	2019-12-10 21:34:45.103643981 +0100
+++ gcc/tree-vect-patterns.c	2019-12-11 18:21:11.678218461 +0100
@@ -2432,14 +2432,12 @@ vect_recog_rotate_pattern (stmt_vec_info
       oprnd0 = def;
     }
 
-  if (dt == vect_external_def
-      && TREE_CODE (oprnd1) == SSA_NAME)
+  if (dt == vect_external_def && TREE_CODE (oprnd1) == SSA_NAME)
     ext_def = vect_get_external_def_edge (vinfo, oprnd1);
 
   def = NULL_TREE;
   scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
-  if (TREE_CODE (oprnd1) == INTEGER_CST
-      || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)
+  if (dt != vect_internal_def || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)
     def = oprnd1;
   else if (def_stmt && gimple_assign_cast_p (def_stmt))
     {
@@ -2454,14 +2452,7 @@ vect_recog_rotate_pattern (stmt_vec_info
     {
       def = vect_recog_temp_ssa_var (type, NULL);
       def_stmt = gimple_build_assign (def, NOP_EXPR, oprnd1);
-      if (ext_def)
-	{
-	  basic_block new_bb
-	    = gsi_insert_on_edge_immediate (ext_def, def_stmt);
-	  gcc_assert (!new_bb);
-	}
-      else
-	append_pattern_def_seq (stmt_vinfo, def_stmt);
+      append_pattern_def_seq (stmt_vinfo, def_stmt);
     }
   stype = TREE_TYPE (def);
   scalar_int_mode smode = SCALAR_INT_TYPE_MODE (stype);
--- gcc/testsuite/gcc.dg/vect/pr92723.c.jj	2019-12-11 18:19:09.944060313 +0100
+++ gcc/testsuite/gcc.dg/vect/pr92723.c	2019-12-11 18:19:09.944060313 +0100
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+
+void
+foo (unsigned long long *x, unsigned long long *y, int z)
+{
+  int i;
+  for (i = 0; i < 1024; i++)
+    x[i] = (y[i] >> z) | (y[i] << (-z & (__SIZEOF_LONG_LONG__ * __CHAR_BIT__ - 1)));
+}


	Jakub
Richard Sandiford Dec. 11, 2019, 5:57 p.m. UTC | #3 
Jakub Jelinek <jakub@redhat.com> writes:
> On Wed, Dec 11, 2019 at 04:52:30PM +0000, Richard Sandiford wrote:
>> WDYT about instead having:
>> 
>>   if (dt != vect_internal_def || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)
>> 
>> and removing the ext_def stuff?  I'd have expected keeping the original
>> operand to always be best for vect_external_def, and it avoids changing
>> the function body during what's supposed to be just an analysis phase.
>
> The ext_def stuff is needed in any case, for the -amount & mask expression.
> If all you mean is following, then I think it should work and can
> bootstrap/regtest it tonight (though just on x86_64/i686):

Yeah, this is what I meant, sorry for the vague description.

> 2019-12-11  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR target/92723
> 	* tree-vect-patterns.c (vect_recog_rotate_pattern): If dt is not
> 	vect_internal_def, use oprnd1 as is, without trying to cast it.
> 	Formatting fix.
>
> 	* gcc.dg/vect/pr92723.c: New test.

OK if it passes testing.

Thanks,
Richard

>
> --- gcc/tree-vect-patterns.c.jj	2019-12-10 21:34:45.103643981 +0100
> +++ gcc/tree-vect-patterns.c	2019-12-11 18:21:11.678218461 +0100
> @@ -2432,14 +2432,12 @@ vect_recog_rotate_pattern (stmt_vec_info
>        oprnd0 = def;
>      }
>  
> -  if (dt == vect_external_def
> -      && TREE_CODE (oprnd1) == SSA_NAME)
> +  if (dt == vect_external_def && TREE_CODE (oprnd1) == SSA_NAME)
>      ext_def = vect_get_external_def_edge (vinfo, oprnd1);
>  
>    def = NULL_TREE;
>    scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
> -  if (TREE_CODE (oprnd1) == INTEGER_CST
> -      || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)
> +  if (dt != vect_internal_def || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)
>      def = oprnd1;
>    else if (def_stmt && gimple_assign_cast_p (def_stmt))
>      {
> @@ -2454,14 +2452,7 @@ vect_recog_rotate_pattern (stmt_vec_info
>      {
>        def = vect_recog_temp_ssa_var (type, NULL);
>        def_stmt = gimple_build_assign (def, NOP_EXPR, oprnd1);
> -      if (ext_def)
> -	{
> -	  basic_block new_bb
> -	    = gsi_insert_on_edge_immediate (ext_def, def_stmt);
> -	  gcc_assert (!new_bb);
> -	}
> -      else
> -	append_pattern_def_seq (stmt_vinfo, def_stmt);
> +      append_pattern_def_seq (stmt_vinfo, def_stmt);
>      }
>    stype = TREE_TYPE (def);
>    scalar_int_mode smode = SCALAR_INT_TYPE_MODE (stype);
> --- gcc/testsuite/gcc.dg/vect/pr92723.c.jj	2019-12-11 18:19:09.944060313 +0100
> +++ gcc/testsuite/gcc.dg/vect/pr92723.c	2019-12-11 18:19:09.944060313 +0100
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +
> +void
> +foo (unsigned long long *x, unsigned long long *y, int z)
> +{
> +  int i;
> +  for (i = 0; i < 1024; i++)
> +    x[i] = (y[i] >> z) | (y[i] << (-z & (__SIZEOF_LONG_LONG__ * __CHAR_BIT__ - 1)));
> +}
>
>
> 	Jakub
diff mbox series

Patch

--- gcc/tree-vect-patterns.c.jj	2019-12-09 11:12:29.983288823 +0100
+++ gcc/tree-vect-patterns.c	2019-12-10 16:30:59.922177911 +0100
@@ -2242,6 +2242,7 @@  vect_recog_rotate_pattern (stmt_vec_info
   optab optab1, optab2;
   edge ext_def = NULL;
   bool bswap16_p = false;
+  bool scalar_shift_p = false;
 
   if (is_gimple_assign (last_stmt))
     {
@@ -2420,6 +2421,7 @@  vect_recog_rotate_pattern (stmt_vec_info
 	  || !optab2
 	  || optab_handler (optab2, TYPE_MODE (vectype)) == CODE_FOR_nothing)
 	return NULL;
+      scalar_shift_p = true;
     }
 
   *type_out = vectype;
@@ -2439,7 +2441,8 @@  vect_recog_rotate_pattern (stmt_vec_info
   def = NULL_TREE;
   scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
   if (TREE_CODE (oprnd1) == INTEGER_CST
-      || TYPE_MODE (TREE_TYPE (oprnd1)) == mode)
+      || TYPE_MODE (TREE_TYPE (oprnd1)) == mode
+      || scalar_shift_p)
     def = oprnd1;
   else if (def_stmt && gimple_assign_cast_p (def_stmt))
     {
--- gcc/testsuite/gcc.dg/vect/pr92723.c.jj	2019-12-10 16:37:09.924375698 +0100
+++ gcc/testsuite/gcc.dg/vect/pr92723.c	2019-12-10 16:37:21.823189103 +0100
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+
+void
+foo (unsigned long long *x, unsigned long long *y, int z)
+{
+  int i;
+  for (i = 0; i < 1024; i++)
+    x[i] = (y[i] >> z) | (y[i] << (-z & (__SIZEOF_LONG_LONG__ * __CHAR_BIT__ - 1)));
+}