[PATCH PR96195] aarch64: ICE during GIMPLE pass:vect

[PATCH PR96195] aarch64: ICE during GIMPLE pass:vect

[yangyang (ET)]

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

Hi, 

	This is a simple fix for PR96195.

	For the test case, GCC generates the following gimple statement in pass_vect:

	  vect__21.16_58 = zp.simdclone.2 (vect_vec_iv_.15_56);

	The mode of vect__21.16_58 is VNx2SI while the mode of zp.simdclone.2 (vect_vec_iv_.15_56) is V4SI, resulting in the crash.

	In vectorizable_simd_clone_call, type compatibility is handled based on the number of elements and the type compatibility of elements, which is not enough. 
	This patch add VIEW_CONVERT_EXPRs if the arguments types and return type of simd clone function are distinct with the vectype of stmt.

	Added one testcase for this. Bootstrap and tested on both aarch64 and x86 Linux platform, no new regression witnessed.

	Ok for trunk?

Thanks,
Yang Yang


+2020-07-15  Yang Yang  <yangyang305@huawei.com>
+
+       PR tree-optimization/96195
+       * tree-vect-stmts.c (vectorizable_simd_clone_call): Add
+       VIEW_CONVERT_EXPRs if the arguments types and return type
+       of simd clone function are distinct with the vectype of stmt.
+

+2020-07-15  Yang Yang  <yangyang305@huawei.com>
+
+       PR tree-optimization/96195
+       * gcc.target/aarch64/sve/pr96195.c: New test.
+

[-- Attachment #2: pr96195-v1.patch --]
[-- Type: application/octet-stream, Size: 3503 bytes --]

From 4dd96316aacffd11c3a0da8fce87d8cc295e6366 Mon Sep 17 00:00:00 2001
From: y00520163 <yangyang305@huawei.com>
Date: Wed, 15 Jul 2020 19:53:52 +0800
Subject: [PATCH] vect: Fix an ICE in vectorizable_simd_clone_call

In vectorizable_simd_clone_call, type compatibility is handled based on
the number of elements and the type compatibility of elements, which is
not enough. This patch add VIEW_CONVERT_EXPRs if the arguments types
and return type of simd clone function are distinct with the vectype of
stmt.

2020-07-15  Yang Yang <yangyang305@huawei.com>

gcc/ChangeLog:

	* tree-vect-stmts.c (vectorizable_simd_clone_call): Add
	VIEW_CONVERT_EXPRs if the arguments types and return type
	of simd clone function are distinct with the vectype of stmt.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/sve/pr96195.c: New test.
---
 .../gcc.target/aarch64/sve/pr96195.c          | 17 +++++++++++++
 gcc/tree-vect-stmts.c                         | 25 ++++++++++++++++---
 2 files changed, 39 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/pr96195.c

diff --git a/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c b/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c
new file mode 100644
index 00000000000..d879efda5c8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve/pr96195.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O1 -fopenmp-simd -ftree-vectorize -msve-vector-bits=128" } */
+
+int by;
+
+#pragma omp declare simd
+int
+zp (int);
+
+void
+qh (int oh)
+{
+#pragma omp simd
+  for (by = 0; by < oh; ++by)
+    by = zp (by);
+}
+
diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
index 6730cae8085..8301494fa41 100644
--- a/gcc/tree-vect-stmts.c
+++ b/gcc/tree-vect-stmts.c
@@ -4108,7 +4108,20 @@ vectorizable_simd_clone_call (vec_info *vinfo, stmt_vec_info stmt_info,
 						  vec_oprnd0);
 			}
 		      if (k == 1)
-			vargs.safe_push (vec_oprnd0);
+			if (!useless_type_conversion_p (TREE_TYPE (vec_oprnd0),
+						       atype))
+			  {
+			    vec_oprnd0
+			      = build1 (VIEW_CONVERT_EXPR, atype, vec_oprnd0);
+			    gassign *new_stmt
+			      = gimple_build_assign (make_ssa_name (atype),
+						       vec_oprnd0);
+			    vect_finish_stmt_generation (vinfo, stmt_info,
+							 new_stmt, gsi);
+			    vargs.safe_push (gimple_assign_lhs (new_stmt));
+			  }
+			else
+			  vargs.safe_push (vec_oprnd0);
 		      else
 			{
 			  vec_oprnd0 = build_constructor (atype, ctor_elts);
@@ -4204,8 +4217,7 @@ vectorizable_simd_clone_call (vec_info *vinfo, stmt_vec_info stmt_info,
 	  gcc_assert (ratype || simd_clone_subparts (rtype) == nunits);
 	  if (ratype)
 	    new_temp = create_tmp_var (ratype);
-	  else if (simd_clone_subparts (vectype)
-		   == simd_clone_subparts (rtype))
+	  else if (useless_type_conversion_p (vectype, rtype))
 	    new_temp = make_ssa_name (vec_dest, new_call);
 	  else
 	    new_temp = make_ssa_name (rtype, new_call);
@@ -4293,6 +4305,13 @@ vectorizable_simd_clone_call (vec_info *vinfo, stmt_vec_info stmt_info,
 	      vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
 	      vect_clobber_variable (vinfo, stmt_info, gsi, new_temp);
 	    }
+	  else if (!useless_type_conversion_p (vectype, rtype))
+	    {
+	      vec_oprnd0 = build1 (VIEW_CONVERT_EXPR, vectype, new_temp);
+	      new_stmt
+		= gimple_build_assign (make_ssa_name (vec_dest), vec_oprnd0);
+	      vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
+	    }
 	}
 
       if (j == 0)
-- 
2.19.1

Re: [PATCH PR96195] aarch64: ICE during GIMPLE pass:vect

[Richard Sandiford]

Sorry for the slow reply.

"yangyang (ET)" <yangyang305@huawei.com> writes:
> Hi, 
>
> 	This is a simple fix for PR96195.
>
> 	For the test case, GCC generates the following gimple statement in pass_vect:
>
> 	  vect__21.16_58 = zp.simdclone.2 (vect_vec_iv_.15_56);
>
> 	The mode of vect__21.16_58 is VNx2SI while the mode of zp.simdclone.2 (vect_vec_iv_.15_56) is V4SI, resulting in the crash.
>
> 	In vectorizable_simd_clone_call, type compatibility is handled based on the number of elements and the type compatibility of elements, which is not enough. 
> 	This patch add VIEW_CONVERT_EXPRs if the arguments types and return type of simd clone function are distinct with the vectype of stmt.
>
> 	Added one testcase for this. Bootstrap and tested on both aarch64 and x86 Linux platform, no new regression witnessed.

I agree this looks correct as far as the target-independent interface
goes.  However, the underlying problem is that we haven't yet added
support for SVE omp simd functions.  What should happen for the testcase
is that we assume both SVE and Advanced SIMD versions of zp exist and
call the SVE version instead of the Advanced SIMD version.

There again, for little-endian -msve-vector-bits=128 there should
be no overhead with using the Advanced SIMD version, and big-endian
-msve-vector-bits=128 is equivalent to -msve-vector-bits=scalable.

Things would get more interesting for:

  #pragma omp declare simd simdlen(8)
  int
  zp (int);

and -msve-vector-bits=256, but again, we don't yet support simdlen(8)
for Advanced SIMD.

So all in all, I agree this is the right fix.  Pushed to master with
a minor whitespace fixup for:

> +			    gassign *new_stmt
> +			      = gimple_build_assign (make_ssa_name (atype),
> +						       vec_oprnd0);

…the indentation on this line.

Thanks,
Richard

RE: [PATCH PR96195] aarch64: ICE during GIMPLE pass:vect

[yangyang (ET)]

> -----Original Message-----
> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
> Sent: Tuesday, July 21, 2020 2:49 AM
> To: yangyang (ET) <yangyang305@huawei.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH PR96195] aarch64: ICE during GIMPLE pass:vect
> 
> Sorry for the slow reply.
> 
> "yangyang (ET)" <yangyang305@huawei.com> writes:
> > Hi,
> >
> > 	This is a simple fix for PR96195.
> >
> > 	For the test case, GCC generates the following gimple statement in
> pass_vect:
> >
> > 	  vect__21.16_58 = zp.simdclone.2 (vect_vec_iv_.15_56);
> >
> > 	The mode of vect__21.16_58 is VNx2SI while the mode of zp.simdclone.2
> (vect_vec_iv_.15_56) is V4SI, resulting in the crash.
> >
> > 	In vectorizable_simd_clone_call, type compatibility is handled based on
> the number of elements and the type compatibility of elements, which is not
> enough.
> > 	This patch add VIEW_CONVERT_EXPRs if the arguments types and return
> type of simd clone function are distinct with the vectype of stmt.
> >
> > 	Added one testcase for this. Bootstrap and tested on both aarch64 and
> x86 Linux platform, no new regression witnessed.
> 
> I agree this looks correct as far as the target-independent interface goes.
> However, the underlying problem is that we haven't yet added support for SVE
> omp simd functions.  What should happen for the testcase is that we assume
> both SVE and Advanced SIMD versions of zp exist and call the SVE version
> instead of the Advanced SIMD version.
> 
> There again, for little-endian -msve-vector-bits=128 there should be no
> overhead with using the Advanced SIMD version, and big-endian
> -msve-vector-bits=128 is equivalent to -msve-vector-bits=scalable.
> 
> Things would get more interesting for:
> 
>   #pragma omp declare simd simdlen(8)
>   int
>   zp (int);
> 
> and -msve-vector-bits=256, but again, we don't yet support simdlen(8) for
> Advanced SIMD.
> 
> So all in all, I agree this is the right fix.  Pushed to master with a minor
> whitespace fixup for:
> 
> > +			    gassign *new_stmt
> > +			      = gimple_build_assign (make_ssa_name (atype),
> > +						       vec_oprnd0);
> 
> …the indentation on this line.
> 
> Thanks,
> Richard

Thanks for reviewing and installing the patch. By the way, are there any plans for adding support for SVE omp simd functions in the future?

Re: [PATCH PR96195] aarch64: ICE during GIMPLE pass:vect

[Richard Sandiford]

"yangyang (ET)" <yangyang305@huawei.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
>> Sent: Tuesday, July 21, 2020 2:49 AM
>> To: yangyang (ET) <yangyang305@huawei.com>
>> Cc: gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH PR96195] aarch64: ICE during GIMPLE pass:vect
>> 
>> Sorry for the slow reply.
>> 
>> "yangyang (ET)" <yangyang305@huawei.com> writes:
>> > Hi,
>> >
>> > 	This is a simple fix for PR96195.
>> >
>> > 	For the test case, GCC generates the following gimple statement in
>> pass_vect:
>> >
>> > 	  vect__21.16_58 = zp.simdclone.2 (vect_vec_iv_.15_56);
>> >
>> > 	The mode of vect__21.16_58 is VNx2SI while the mode of zp.simdclone.2
>> (vect_vec_iv_.15_56) is V4SI, resulting in the crash.
>> >
>> > 	In vectorizable_simd_clone_call, type compatibility is handled based on
>> the number of elements and the type compatibility of elements, which is not
>> enough.
>> > 	This patch add VIEW_CONVERT_EXPRs if the arguments types and return
>> type of simd clone function are distinct with the vectype of stmt.
>> >
>> > 	Added one testcase for this. Bootstrap and tested on both aarch64 and
>> x86 Linux platform, no new regression witnessed.
>> 
>> I agree this looks correct as far as the target-independent interface goes.
>> However, the underlying problem is that we haven't yet added support for SVE
>> omp simd functions.  What should happen for the testcase is that we assume
>> both SVE and Advanced SIMD versions of zp exist and call the SVE version
>> instead of the Advanced SIMD version.
>> 
>> There again, for little-endian -msve-vector-bits=128 there should be no
>> overhead with using the Advanced SIMD version, and big-endian
>> -msve-vector-bits=128 is equivalent to -msve-vector-bits=scalable.
>> 
>> Things would get more interesting for:
>> 
>>   #pragma omp declare simd simdlen(8)
>>   int
>>   zp (int);
>> 
>> and -msve-vector-bits=256, but again, we don't yet support simdlen(8) for
>> Advanced SIMD.
>> 
>> So all in all, I agree this is the right fix.  Pushed to master with a minor
>> whitespace fixup for:
>> 
>> > +			    gassign *new_stmt
>> > +			      = gimple_build_assign (make_ssa_name (atype),
>> > +						       vec_oprnd0);
>> 
>> …the indentation on this line.
>> 
>> Thanks,
>> Richard
>
> Thanks for reviewing and installing the patch. By the way, are there any plans for adding support for SVE omp simd functions in the future?

FWIW, I've filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96342
to track this.  I guess the easiest thing would be for anyone who's
planning on working on it to assign themselves.

Thanks,
Richard