[PATCH] openmp-simd-clone: Match shift type

[PATCH] openmp-simd-clone: Match shift type

[Andrew Stubbs]

[-- Attachment #1: Type: text/plain, Size: 284 bytes --]

This patch adjusts the generation of SIMD "inbranch" clones that use 
integer masks to ensure that it vectorizes on amdgcn.

The problem was only that an amdgcn mask is DImode and the shift amount 
was SImode, and the difference causes vectorization to fail.

OK for mainline?

Andrew

[-- Attachment #2: 220729-simd-clone-shift-types.patch --]
[-- Type: text/plain, Size: 1115 bytes --]

openmp-simd-clone: Match shift types

Ensure that both parameters to vector shifts use the same mode.  This is most
important for amdgcn where the masks are DImode.

gcc/ChangeLog:

	* omp-simd-clone.cc (simd_clone_adjust): Convert shift_cnt to match
	the mask type.

diff --git a/gcc/omp-simd-clone.cc b/gcc/omp-simd-clone.cc
index 32649bc3f9a..5d3a90730e7 100644
--- a/gcc/omp-simd-clone.cc
+++ b/gcc/omp-simd-clone.cc
@@ -1305,8 +1305,12 @@ simd_clone_adjust (struct cgraph_node *node)
 				       build_int_cst (TREE_TYPE (iter1), c));
 	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
 	    }
+	  tree shift_cnt_conv = make_ssa_name (TREE_TYPE (mask));
+	  g = gimple_build_assign (shift_cnt_conv,
+				   fold_convert (TREE_TYPE (mask), shift_cnt));
+	  gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
 	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (mask)),
-				   RSHIFT_EXPR, mask, shift_cnt);
+				   RSHIFT_EXPR, mask, shift_cnt_conv);
 	  gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
 	  mask = gimple_assign_lhs (g);
 	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (mask)),

Re: [PATCH] openmp-simd-clone: Match shift type

[Jakub Jelinek]

On Fri, Jul 29, 2022 at 04:53:51PM +0100, Andrew Stubbs wrote:
> This patch adjusts the generation of SIMD "inbranch" clones that use integer
> masks to ensure that it vectorizes on amdgcn.
> 
> The problem was only that an amdgcn mask is DImode and the shift amount was
> SImode, and the difference causes vectorization to fail.
> 
> OK for mainline?
> 
> Andrew

> openmp-simd-clone: Match shift types
> 
> Ensure that both parameters to vector shifts use the same mode.  This is most
> important for amdgcn where the masks are DImode.
> 
> gcc/ChangeLog:
> 
> 	* omp-simd-clone.cc (simd_clone_adjust): Convert shift_cnt to match
> 	the mask type.
> 
> diff --git a/gcc/omp-simd-clone.cc b/gcc/omp-simd-clone.cc
> index 32649bc3f9a..5d3a90730e7 100644
> --- a/gcc/omp-simd-clone.cc
> +++ b/gcc/omp-simd-clone.cc
> @@ -1305,8 +1305,12 @@ simd_clone_adjust (struct cgraph_node *node)
>  				       build_int_cst (TREE_TYPE (iter1), c));
>  	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
>  	    }
> +	  tree shift_cnt_conv = make_ssa_name (TREE_TYPE (mask));
> +	  g = gimple_build_assign (shift_cnt_conv,
> +				   fold_convert (TREE_TYPE (mask), shift_cnt));
> +	  gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);

Doing the fold_convert seems to be a wasted effort to me.
Can't this be done conditional on whether some change is needed at all
and just using gimple_build_assign with NOP_EXPR, so something like:
	  tree shift_cvt_conv = shift_cnt;
	  if (!useless_type_conversion_p (TREE_TYPE (mask),
					  TREE_TYPE (shift_cnt)))
	    {
	      shift_cnt_conv = make_ssa_name (TREE_TYPE (mask));
	      g = gimple_build_assign (shift_cnt_conv, NOP_EXPR, shift_cnt);
	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
	    }

>  	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (mask)),
> -				   RSHIFT_EXPR, mask, shift_cnt);
> +				   RSHIFT_EXPR, mask, shift_cnt_conv);
>  	  gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
>  	  mask = gimple_assign_lhs (g);
>  	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (mask)),

?

	Jakub

Re: [PATCH] openmp-simd-clone: Match shift type

[Andrew Stubbs]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On 29/07/2022 16:59, Jakub Jelinek wrote:
> Doing the fold_convert seems to be a wasted effort to me.
> Can't this be done conditional on whether some change is needed at all
> and just using gimple_build_assign with NOP_EXPR, so something like:

I'm just not familiar enough with this stuff to run fold_convert in my 
head with confidence.

> 	  tree shift_cvt_conv = shift_cnt;
> 	  if (!useless_type_conversion_p (TREE_TYPE (mask),
> 					  TREE_TYPE (shift_cnt)))
> 	    {
> 	      shift_cnt_conv = make_ssa_name (TREE_TYPE (mask));
> 	      g = gimple_build_assign (shift_cnt_conv, NOP_EXPR, shift_cnt);
> 	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
> 	    }
> 

Your version gives the same output mine does, at least on amdgcn anyway.

Am I OK to commit this version?

Andrew

[-- Attachment #2: 220729-simd-clone-shift-types-2.patch --]
[-- Type: text/plain, Size: 1281 bytes --]

openmp-simd-clone: Match shift types

Ensure that both parameters to vector shifts use the same mode.  This is most
important for amdgcn where the masks are DImode.

gcc/ChangeLog:

	* omp-simd-clone.cc (simd_clone_adjust): Convert shift_cnt to match
	the mask type.

Co-authored-by: Jakub Jelinek  <jakub@redhat.com>

diff --git a/gcc/omp-simd-clone.cc b/gcc/omp-simd-clone.cc
index 32649bc3f9a..58bd68b129b 100644
--- a/gcc/omp-simd-clone.cc
+++ b/gcc/omp-simd-clone.cc
@@ -1305,8 +1305,16 @@ simd_clone_adjust (struct cgraph_node *node)
 				       build_int_cst (TREE_TYPE (iter1), c));
 	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
 	    }
+	  tree shift_cnt_conv = shift_cnt;
+	  if (!useless_type_conversion_p (TREE_TYPE (mask),
+					  TREE_TYPE (shift_cnt)))
+	    {
+	      shift_cnt_conv = make_ssa_name (TREE_TYPE (mask));
+	      g = gimple_build_assign (shift_cnt_conv, NOP_EXPR, shift_cnt);
+	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
+	    }
 	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (mask)),
-				   RSHIFT_EXPR, mask, shift_cnt);
+				   RSHIFT_EXPR, mask, shift_cnt_conv);
 	  gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
 	  mask = gimple_assign_lhs (g);
 	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (mask)),

Re: [PATCH] openmp-simd-clone: Match shift type

[Jakub Jelinek]

On Fri, Jul 29, 2022 at 06:03:18PM +0100, Andrew Stubbs wrote:
> On 29/07/2022 16:59, Jakub Jelinek wrote:
> > Doing the fold_convert seems to be a wasted effort to me.
> > Can't this be done conditional on whether some change is needed at all
> > and just using gimple_build_assign with NOP_EXPR, so something like:
> 
> I'm just not familiar enough with this stuff to run fold_convert in my head
> with confidence.

The thing with fold_convert is that if some conversion is needed (and
fold_convert actually is strict, so even if the conversion is useless
but the type isn't exactly the same) it creates a NOP_EXPR around the
argument, and then gimple_build_assign notices it should create a NOP_EXPR
assign rhs op and just uses the argument of NOP_EXPR, where the NOP_EXPR
will be GC later.
Plus, if the conversion isn't needed, it creates an extra assignment that
will be only later in some other pass optimized away.
> 
> > 	  tree shift_cvt_conv = shift_cnt;
> > 	  if (!useless_type_conversion_p (TREE_TYPE (mask),
> > 					  TREE_TYPE (shift_cnt)))
> > 	    {
> > 	      shift_cnt_conv = make_ssa_name (TREE_TYPE (mask));
> > 	      g = gimple_build_assign (shift_cnt_conv, NOP_EXPR, shift_cnt);
> > 	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
> > 	    }
> > 
> 
> Your version gives the same output mine does, at least on amdgcn anyway.
> 
> Am I OK to commit this version?

Yes, thanks.

> openmp-simd-clone: Match shift types
> 
> Ensure that both parameters to vector shifts use the same mode.  This is most
> important for amdgcn where the masks are DImode.
> 
> gcc/ChangeLog:
> 
> 	* omp-simd-clone.cc (simd_clone_adjust): Convert shift_cnt to match
> 	the mask type.
> 
> Co-authored-by: Jakub Jelinek  <jakub@redhat.com>
> 
> diff --git a/gcc/omp-simd-clone.cc b/gcc/omp-simd-clone.cc
> index 32649bc3f9a..58bd68b129b 100644
> --- a/gcc/omp-simd-clone.cc
> +++ b/gcc/omp-simd-clone.cc
> @@ -1305,8 +1305,16 @@ simd_clone_adjust (struct cgraph_node *node)
>  				       build_int_cst (TREE_TYPE (iter1), c));
>  	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
>  	    }
> +	  tree shift_cnt_conv = shift_cnt;
> +	  if (!useless_type_conversion_p (TREE_TYPE (mask),
> +					  TREE_TYPE (shift_cnt)))
> +	    {
> +	      shift_cnt_conv = make_ssa_name (TREE_TYPE (mask));
> +	      g = gimple_build_assign (shift_cnt_conv, NOP_EXPR, shift_cnt);
> +	      gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
> +	    }
>  	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (mask)),
> -				   RSHIFT_EXPR, mask, shift_cnt);
> +				   RSHIFT_EXPR, mask, shift_cnt_conv);
>  	  gsi_insert_after (&gsi, g, GSI_CONTINUE_LINKING);
>  	  mask = gimple_assign_lhs (g);
>  	  g = gimple_build_assign (make_ssa_name (TREE_TYPE (mask)),


	Jakub