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

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

[duanbo (C)]

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

Hi, 

This is a fix for PR96757.
Before vect pass, the main statements of the test case will be optimized like：
	_1 = d_10(D) > 3;
	_2 = a_11(D) > m_12(D);
	_18 = _1 > _2 ? _26 : 0; 
At the beginning of vectorization analysis, function vect_recog_mask_conversion_pattern 
processed the gimple  _18 = _1 > _2 ? _15 : 0, and produce:
	patt_17 = _1 > _2
	patt_3 = patt_17 ? _15 : 0
it didn't consider the situation that  _1, _2's vectype is different, which leading to an ICE in 
some special  mode , like V4HImode for this case.

This patch added the identification and handling for this situation in vect_recog_mask_conversion_pattern. 
With that _18 = _1 > _2 ? _15 : 0 will be transformed to:
	patt_3 = (<signed-boolean:16>) _2;
	patt_17 = _1 > patt_3;
	patt_20 = patt_17 ? _26 : 0;

More details in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96757.
Bootstrap and tested on both aarch64 and x86 Linux platform, no new regression witnessed.
Ok for trunk?

Thanks, 
Duan Bo
	

[-- Attachment #2: pr95767.patch --]
[-- Type: application/octet-stream, Size: 3240 bytes --]

From 421dc953cecf09193273827f272a0d185c936f91 Mon Sep 17 00:00:00 2001
From: Duan bo <duanbo3@huawei.com>
Date: Wed, 26 Aug 2020 03:53:32 -0400
Subject: [PATCH] vect: Fix an ICE in vect_recog_mask_conversion_pattern

When processing the cond expression, vect_recog_mask_conversion_pattern
doesn't consider the situation that two operands of rhs1 are different
vectypes, leading to a vect ICE. This patch adds the identification and
handling of the situation to fix the problem.

gcc/ChangeLog:

	PR target/96757
	* tree-vect-patterns.c (vect_recog_mask_conversion_pattern): Add
	the identification and handling of the dropped situation in the cond
	expression processing phase.

gcc/testsuite/ChangeLog:

	PR target/96757
	* gcc.target/aarch64/pr96757.c: New test.
---
 gcc/testsuite/gcc.target/aarch64/pr96757.c | 24 +++++++++++++++
 gcc/tree-vect-patterns.c                   | 34 ++++++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr96757.c

diff --git a/gcc/testsuite/gcc.target/aarch64/pr96757.c b/gcc/testsuite/gcc.target/aarch64/pr96757.c
new file mode 100644
index 00000000000..4bca0fa1680
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr96757.c
@@ -0,0 +1,24 @@
+/* PR target/96757 */
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+short 
+fun1(short i, short j)
+{ 
+  return i * j; 
+}
+
+int 
+fun(int a, int b, int c) 
+{
+  int *v, z, k, m;
+  short f, d;
+  for (int i=0; i<c; i++) 
+  {
+    f= 4 <= d;
+    k= a > m;
+    z = f > k;
+    *v += fun1(z,b);
+  }
+}
+
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 03d50ec5c90..00dfaa6b395 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -4395,6 +4395,40 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
 	{
 	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
 	  pattern_stmt = gimple_build_assign (tmp, rhs1);
+	  tree rhs1_op0 = TREE_OPERAND (rhs1, 0);
+	  tree rhs1_op1 = TREE_OPERAND (rhs1, 1);
+	  if (rhs1_op0 && rhs1_op1
+	      && (TREE_CODE (TREE_TYPE (rhs1_op0)) == BOOLEAN_TYPE)
+	      && (TREE_CODE (TREE_TYPE (rhs1_op1)) == BOOLEAN_TYPE))
+	    {
+	      tree rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
+	      tree rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
+	      enum tree_code rhs1_code = gimple_assign_rhs_code (pattern_stmt);
+	      if (rhs1_op0_type && rhs1_op1_type
+		  && (!(TYPE_PRECISION (rhs1_op0_type)
+			== TYPE_PRECISION (rhs1_op1_type))))
+		{
+		  if (TYPE_PRECISION (rhs1_op0_type)
+		      < TYPE_PRECISION (rhs1_op1_type))
+		    {
+		      vectype2
+			= get_mask_type_for_scalar_type (vinfo, rhs1_op0_type);
+		      if (vectype2)
+			rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
+							  vectype2, stmt_vinfo);
+		    }
+		  else
+		    {
+		      vectype2
+			= get_mask_type_for_scalar_type (vinfo, rhs1_op1_type);
+		      if (vectype2)
+			rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
+							  vectype2, stmt_vinfo);
+		    }
+		  pattern_stmt = gimple_build_assign (tmp, rhs1_code,
+				 		      rhs1_op0, rhs1_op1);
+		}
+	    }
 	  rhs1 = tmp;
 	  append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt, vectype2,
 				  rhs1_type);
-- 
2.19.1

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

[Richard Sandiford]

"duanbo (C)" <duanbo3@huawei.com> writes:
> @@ -4395,6 +4395,40 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
>  	{
>  	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
>  	  pattern_stmt = gimple_build_assign (tmp, rhs1);
> +	  tree rhs1_op0 = TREE_OPERAND (rhs1, 0);
> +	  tree rhs1_op1 = TREE_OPERAND (rhs1, 1);

I think we also need to handle this case when picking the vector
types and deciding whether a pattern is needed.  Otherwise it would
be possible for rhs1_op1 to be the only input that requires a different
mask, and we'd wrongly avoid creating a conversion for it.

> +	  if (rhs1_op0 && rhs1_op1
> +	      && (TREE_CODE (TREE_TYPE (rhs1_op0)) == BOOLEAN_TYPE)
> +	      && (TREE_CODE (TREE_TYPE (rhs1_op1)) == BOOLEAN_TYPE))

Minor formatting nit, but the brackets around the == are redundant
(but are still kept for multiline comparisons like the == below).

> +	    {
> +	      tree rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
> +	      tree rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
> +	      enum tree_code rhs1_code = gimple_assign_rhs_code (pattern_stmt);
> +	      if (rhs1_op0_type && rhs1_op1_type
> +		  && (!(TYPE_PRECISION (rhs1_op0_type)
> +			== TYPE_PRECISION (rhs1_op1_type))))

More obvious as != rather than !(… == …).

> +		{
> +		  if (TYPE_PRECISION (rhs1_op0_type)
> +		      < TYPE_PRECISION (rhs1_op1_type))
> +		    {
> +		      vectype2
> +			= get_mask_type_for_scalar_type (vinfo, rhs1_op0_type);
> +		      if (vectype2)
> +			rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> +							  vectype2, stmt_vinfo);

It isn't safe to ignore null returns like this.  We should bail out
instead.

> +		    }
> +		  else
> +		    {
> +		      vectype2
> +			= get_mask_type_for_scalar_type (vinfo, rhs1_op1_type);
> +		      if (vectype2)
> +			rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> +							  vectype2, stmt_vinfo);
> +		    }

This seems to go for the narrower of the two precisions.  Are you sure
that's always the optimal choice?  Gut instinct says that we should
move in the direction of vectype1, since that's what the result will
be converted to.  E.g. maybe:

- if both precisions are less than vectype1's precision or both are
  more than vectype1's precision, pick the closest to vectype1.

- otherwise, convert both inputs to vectype1 individually, rather than
  converting the result to vectype1.

I admit I haven't thought about it much though.

> +		  pattern_stmt = gimple_build_assign (tmp, rhs1_code,
> +				 		      rhs1_op0, rhs1_op1);
> +		}

This statement is redundant with the one created above.  I think instead
we should create the statement this way in all cases, even if rhs1_op0
and rhs1_op1 don't change.  That's effectively what the:

  gimple_build_assign (tmp, rhs1)

does anyway.

I'm going to be away next week so my next reply might be even slower
than usual, sorry.  Would be happy for someone else to review in the
meantime though. :-)

Thanks,
Richard

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

[duanbo (C)]

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



> -----Original Message-----
> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
> Sent: Saturday, August 29, 2020 2:31 AM
> To: duanbo (C) <duanbo3@huawei.com>
> Cc: GCC Patches <gcc-patches@gcc.gnu.org>; rguenther@suse.de
> Subject: Re: [PATCH PR96757] aarch64: ICE during GIMPLE pass: vect
> 
> "duanbo (C)" <duanbo3@huawei.com> writes:
> > @@ -4395,6 +4395,40 @@ vect_recog_mask_conversion_pattern
> (vec_info *vinfo,
> >  	{
> >  	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
> >  	  pattern_stmt = gimple_build_assign (tmp, rhs1);
> > +	  tree rhs1_op0 = TREE_OPERAND (rhs1, 0);
> > +	  tree rhs1_op1 = TREE_OPERAND (rhs1, 1);
> 
> I think we also need to handle this case when picking the vector types and
> deciding whether a pattern is needed.  Otherwise it would be possible for
> rhs1_op1 to be the only input that requires a different mask, and we'd
> wrongly avoid creating a conversion for it.
> 
> > +	  if (rhs1_op0 && rhs1_op1
> > +	      && (TREE_CODE (TREE_TYPE (rhs1_op0)) == BOOLEAN_TYPE)
> > +	      && (TREE_CODE (TREE_TYPE (rhs1_op1)) == BOOLEAN_TYPE))
> 
> Minor formatting nit, but the brackets around the == are redundant (but are
> still kept for multiline comparisons like the == below).
> 
> > +	    {
> > +	      tree rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
> > +	      tree rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
> > +	      enum tree_code rhs1_code = gimple_assign_rhs_code
> (pattern_stmt);
> > +	      if (rhs1_op0_type && rhs1_op1_type
> > +		  && (!(TYPE_PRECISION (rhs1_op0_type)
> > +			== TYPE_PRECISION (rhs1_op1_type))))
> 
> More obvious as != rather than !(… == …).
> 
> > +		{
> > +		  if (TYPE_PRECISION (rhs1_op0_type)
> > +		      < TYPE_PRECISION (rhs1_op1_type))
> > +		    {
> > +		      vectype2
> > +			= get_mask_type_for_scalar_type (vinfo,
> rhs1_op0_type);
> > +		      if (vectype2)
> > +			rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> > +							  vectype2,
> stmt_vinfo);
> 
> It isn't safe to ignore null returns like this.  We should bail out instead.
> 
> > +		    }
> > +		  else
> > +		    {
> > +		      vectype2
> > +			= get_mask_type_for_scalar_type (vinfo,
> rhs1_op1_type);
> > +		      if (vectype2)
> > +			rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> > +							  vectype2,
> stmt_vinfo);
> > +		    }
> 
> This seems to go for the narrower of the two precisions.  Are you sure that's
> always the optimal choice?  Gut instinct says that we should move in the
> direction of vectype1, since that's what the result will be converted to.  E.g.
> maybe:
> 
> - if both precisions are less than vectype1's precision or both are
>   more than vectype1's precision, pick the closest to vectype1.
> 
> - otherwise, convert both inputs to vectype1 individually, rather than
>   converting the result to vectype1.
> 
> I admit I haven't thought about it much though.
> 
> > +		  pattern_stmt = gimple_build_assign (tmp, rhs1_code,
> > +				 		      rhs1_op0, rhs1_op1);
> > +		}
> 
> This statement is redundant with the one created above.  I think instead we
> should create the statement this way in all cases, even if rhs1_op0 and
> rhs1_op1 don't change.  That's effectively what the:
> 
>   gimple_build_assign (tmp, rhs1)
> 
> does anyway.
> 
> I'm going to be away next week so my next reply might be even slower than
> usual, sorry.  Would be happy for someone else to review in the meantime
> though. :-)
> 
> Thanks,
> Richard

Sorry for the late reply.
Thanks for your suggestions. I have modified accordingly.
Attached please find the v1 patch. 
Bootstrap and tested on both aarch64 and x86 Linux platform, no new regression witnessed.
Ok for trunk?

Thanks,
Duan bo



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

From 4db92f4164b134e86dc03a90dc14e670efa119f2 Mon Sep 17 00:00:00 2001
From: Duan bo <duanbo3@huawei.com>
Date: Fri, 18 Sep 2020 22:38:49 -0400
Subject: [PATCH] vect: Fix an ICE in vect_recog_mask_conversion_pattern

When processing the cond expression, vect_recog_mask_conversion_pattern
doesn't consider the situation that two operands of rhs1 are different
vectypes, leading to a vect ICE. This patch adds the identification and
handling of the situation to fix the problem.

gcc/ChangeLog:

	PR target/96757
	* tree-vect-patterns.c (vect_recog_mask_conversion_pattern): Add
	the identification and handling of the dropped situation in the
	cond expression processing phase.

gcc/testsuite/ChangeLog:

	PR target/96757
	* gcc.target/aarch64/pr96757.c: New test.
---
 gcc/testsuite/gcc.target/aarch64/pr96757.c | 23 ++++++
 gcc/tree-vect-patterns.c                   | 91 ++++++++++++++++++++--
 2 files changed, 107 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr96757.c

diff --git a/gcc/testsuite/gcc.target/aarch64/pr96757.c b/gcc/testsuite/gcc.target/aarch64/pr96757.c
new file mode 100644
index 00000000000..122e39dca0e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr96757.c
@@ -0,0 +1,23 @@
+/* PR target/96757 */
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+short 
+fun1(short i, short j)
+{ 
+  return i * j; 
+}
+
+int 
+fun(int a, int b, int c) 
+{
+  int *v, z, k, m;
+  short f, d;
+  for (int i=0; i<c; i++) 
+  {
+    f= 4 <= d;
+    k= a > m;
+    z = f > k;
+    *v += fun1(z,b);
+  }
+}
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index db45740da3c..b7c6593087c 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -4241,6 +4241,7 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
   tree lhs = NULL_TREE, rhs1, rhs2, tmp, rhs1_type, rhs2_type;
   tree vectype1, vectype2;
   stmt_vec_info pattern_stmt_info;
+  tree rhs1_op0, rhs1_op1, rhs1_op0_type, rhs1_op1_type;
 
   /* Check for MASK_LOAD ans MASK_STORE calls requiring mask conversion.  */
   if (is_gimple_call (last_stmt)
@@ -4317,7 +4318,8 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
   if (rhs_code == COND_EXPR)
     {
       vectype1 = get_vectype_for_scalar_type (vinfo, TREE_TYPE (lhs));
-
+      if (!vectype1)
+	return NULL;
       if (TREE_CODE (rhs1) == SSA_NAME)
 	{
 	  rhs1_type = integer_type_for_mask (rhs1, vinfo);
@@ -4340,16 +4342,91 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
 
 	     it is better for b1 and b2 to use the mask type associated
 	     with int elements rather bool (byte) elements.  */
-	  rhs1_type = integer_type_for_mask (TREE_OPERAND (rhs1, 0), vinfo);
-	  if (!rhs1_type)
-	    rhs1_type = TREE_TYPE (TREE_OPERAND (rhs1, 0));
+	  rhs1_op0 = TREE_OPERAND (rhs1, 0);
+	  rhs1_op1 = TREE_OPERAND (rhs1, 1);
+	  if (!rhs1_op0 || !rhs1_op1)
+	    return NULL;
+	  rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
+	  rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
+
+	  if (!rhs1_op0_type && !rhs1_op1_type)
+	    {
+	      rhs1_type = TREE_TYPE (rhs1_op0);
+	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
+	    }
+	  else if (!rhs1_op0_type && rhs1_op1_type)
+	    {
+	      rhs1_type = TREE_TYPE (rhs1_op0);
+	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
+	      if (!vectype2)
+		return NULL;
+	      rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
+						vectype2, stmt_vinfo);
+	    }
+	  else if (rhs1_op0_type && !rhs1_op1_type)
+	    {
+	      rhs1_type = TREE_TYPE (rhs1_op1);
+	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
+	      if (!vectype2)
+		return NULL;
+	      rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
+						vectype2, stmt_vinfo);
+	    }
+	  else if (TYPE_PRECISION (rhs1_op0_type)
+		   != TYPE_PRECISION (rhs1_op1_type))
+	    {
+	      int tmp1 = (int)TYPE_PRECISION (rhs1_op0_type)
+			 - (int)TYPE_PRECISION (TREE_TYPE (lhs));
+	      int tmp2 = (int)TYPE_PRECISION (rhs1_op1_type)
+			 - (int)TYPE_PRECISION (TREE_TYPE (lhs));
+	      if ((tmp1 > 0 && tmp2 > 0)||(tmp1 < 0 && tmp2 < 0))
+		{
+		  if (abs (tmp1) > abs (tmp2))
+		    {
+		      vectype2 = get_mask_type_for_scalar_type (vinfo,
+								rhs1_op1_type);
+		      if (!vectype2)
+			return NULL;
+		      rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
+							vectype2, stmt_vinfo);
+		    }
+		  else
+		    {
+		      vectype2 = get_mask_type_for_scalar_type (vinfo,
+								rhs1_op0_type);
+		      if (!vectype2)
+			return NULL;
+		      rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
+							vectype2, stmt_vinfo);
+		    }
+		  rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);
+		}
+	      else
+		{
+		  rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
+						    vectype1, stmt_vinfo);
+		  rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
+						    vectype1, stmt_vinfo);
+		  rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);
+		  if (!rhs1_type)
+		    return NULL;
+		  vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
+		}
+	    }
+	  else
+	    {
+	      rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);
+	      if (!rhs1_type)
+		return NULL;
+	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
+	    }
+	  tmp = build2 (TREE_CODE (rhs1), TREE_TYPE (rhs1), rhs1_op0, rhs1_op1);
+	  rhs1 = tmp;
 	}
       else
 	return NULL;
 
-      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
-
-      if (!vectype1 || !vectype2)
+      if (!vectype2)
 	return NULL;
 
       /* Continue if a conversion is needed.  Also continue if we have
-- 
2.19.1

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

[Richard Sandiford]

Hi,

"duanbo (C)" <duanbo3@huawei.com> writes:
> Sorry for the late reply.

My time to apologise for the late reply.

> Thanks for your suggestions. I have modified accordingly.
> Attached please find the v1 patch. 

Thanks, the logic to choose which precision we pick looks good.
But I think the build_mask_conversions should be deferred until
after we've decided to make the transform.  So…

> @@ -4340,16 +4342,91 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
>  
>  	     it is better for b1 and b2 to use the mask type associated
>  	     with int elements rather bool (byte) elements.  */
> -	  rhs1_type = integer_type_for_mask (TREE_OPERAND (rhs1, 0), vinfo);
> -	  if (!rhs1_type)
> -	    rhs1_type = TREE_TYPE (TREE_OPERAND (rhs1, 0));
> +	  rhs1_op0 = TREE_OPERAND (rhs1, 0);
> +	  rhs1_op1 = TREE_OPERAND (rhs1, 1);
> +	  if (!rhs1_op0 || !rhs1_op1)
> +	    return NULL;
> +	  rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
> +	  rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
> +
> +	  if (!rhs1_op0_type && !rhs1_op1_type)
> +	    {
> +	      rhs1_type = TREE_TYPE (rhs1_op0);
> +	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);

…here we should just be able to set rhs1_type, and leave vectype2
to the code below.

> +	    }
> +	  else if (!rhs1_op0_type && rhs1_op1_type)
> +	    {
> +	      rhs1_type = TREE_TYPE (rhs1_op0);
> +	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
> +	      if (!vectype2)
> +		return NULL;
> +	      rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> +						vectype2, stmt_vinfo);
> +	    }
> +	  else if (rhs1_op0_type && !rhs1_op1_type)
> +	    {
> +	      rhs1_type = TREE_TYPE (rhs1_op1);
> +	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
> +	      if (!vectype2)
> +		return NULL;
> +	      rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> +						vectype2, stmt_vinfo);

Same for these two.

> +	    }
> +	  else if (TYPE_PRECISION (rhs1_op0_type)
> +		   != TYPE_PRECISION (rhs1_op1_type))
> +	    {
> +	      int tmp1 = (int)TYPE_PRECISION (rhs1_op0_type)
> +			 - (int)TYPE_PRECISION (TREE_TYPE (lhs));
> +	      int tmp2 = (int)TYPE_PRECISION (rhs1_op1_type)
> +			 - (int)TYPE_PRECISION (TREE_TYPE (lhs));
> +	      if ((tmp1 > 0 && tmp2 > 0)||(tmp1 < 0 && tmp2 < 0))

Minor formatting nit, sorry, but: GCC style is to put a space after
(int) and on either side of ||.

Might be good to use the same numbering as the operands: tmp0 and tmp1
instead of tmp1 and tmp2.

> +		{
> +		  if (abs (tmp1) > abs (tmp2))
> +		    {
> +		      vectype2 = get_mask_type_for_scalar_type (vinfo,
> +								rhs1_op1_type);
> +		      if (!vectype2)
> +			return NULL;
> +		      rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> +							vectype2, stmt_vinfo);
> +		    }
> +		  else
> +		    {
> +		      vectype2 = get_mask_type_for_scalar_type (vinfo,
> +								rhs1_op0_type);
> +		      if (!vectype2)
> +			return NULL;
> +		      rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> +							vectype2, stmt_vinfo);
> +		    }
> +		  rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);

Here I think we can just go with rhs1_type = rhs1_op1_type if
abs (tmp1) > abs (tmp2) (i.e. op1 is closer to the final type
than op0) and rhs1_op0_type otherwise.

> +		}
> +	      else
> +		{
> +		  rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> +						    vectype1, stmt_vinfo);
> +		  rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> +						    vectype1, stmt_vinfo);
> +		  rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);
> +		  if (!rhs1_type)
> +		    return NULL;
> +		  vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);

and here I think rhs1_type can be:

  build_nonstandard_integer_type (TYPE_PRECISION (lhs_type), 1);

> +		}
> +	    }
> +	  else
> +	    {
> +	      rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);
> +	      if (!rhs1_type)
> +		return NULL;
> +	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);

Here either rhs1_op0_type or rhs1_op1_type should be OK.

> +	    }
> +	  tmp = build2 (TREE_CODE (rhs1), TREE_TYPE (rhs1), rhs1_op0, rhs1_op1);
> +	  rhs1 = tmp;
>  	}
>        else
>  	return NULL;
>  
> -      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
> -
> -      if (!vectype1 || !vectype2)
> +      if (!vectype2)
>  	return NULL;
>  
>        /* Continue if a conversion is needed.  Also continue if we have

With those changes, I think we would need to extend:

      /* Continue if a conversion is needed.  Also continue if we have
	 a comparison whose vector type would normally be different from
	 VECTYPE2 when considered in isolation.  In that case we'll
	 replace the comparison with an SSA name (so that we can record
	 its vector type) and behave as though the comparison was an SSA
	 name from the outset.  */
      if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
		    TYPE_VECTOR_SUBPARTS (vectype2))
	  && (TREE_CODE (rhs1) == SSA_NAME
	      || rhs1_type == TREE_TYPE (TREE_OPERAND (rhs1, 0))))

…the condition on the last line so that it also checks whether
rhs1_op0_type and rhs1_op1_type are null.  If they aren't null,
we need to fall down to the code below.

	return NULL;

And then here we create the build_mask_conversions to vectype2,
like I think you did in the first patch, but reusing the rhs1_op0_type
and rhs1_op1_type from above, and without changing vectype2:

      /* If rhs1 is a comparison we need to move it into a
	 separate statement.  */
      if (TREE_CODE (rhs1) != SSA_NAME)
	{
	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
	  pattern_stmt = gimple_build_assign (tmp, rhs1);
	  rhs1 = tmp;
	  append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt, vectype2,
				  rhs1_type);
	}

Hope that works/makes sense.  Please let me know if it doesn't.

Thanks,
Richard

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

[duanbo (C)]

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



> -----Original Message-----
> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
> Sent: Thursday, September 24, 2020 7:56 PM
> To: duanbo (C) <duanbo3@huawei.com>
> Cc: GCC Patches <gcc-patches@gcc.gnu.org>
> Subject: Re: [PATCH PR96757] aarch64: ICE during GIMPLE pass: vect
> 
> Hi,
> 
> "duanbo (C)" <duanbo3@huawei.com> writes:
> > Sorry for the late reply.
> 
> My time to apologise for the late reply.
> 
> > Thanks for your suggestions. I have modified accordingly.
> > Attached please find the v1 patch.
> 
> Thanks, the logic to choose which precision we pick looks good.
> But I think the build_mask_conversions should be deferred until after we've
> decided to make the transform.  So…
> 
> > @@ -4340,16 +4342,91 @@ vect_recog_mask_conversion_pattern
> (vec_info
> > *vinfo,
> >
> >  	     it is better for b1 and b2 to use the mask type associated
> >  	     with int elements rather bool (byte) elements.  */
> > -	  rhs1_type = integer_type_for_mask (TREE_OPERAND (rhs1, 0),
> vinfo);
> > -	  if (!rhs1_type)
> > -	    rhs1_type = TREE_TYPE (TREE_OPERAND (rhs1, 0));
> > +	  rhs1_op0 = TREE_OPERAND (rhs1, 0);
> > +	  rhs1_op1 = TREE_OPERAND (rhs1, 1);
> > +	  if (!rhs1_op0 || !rhs1_op1)
> > +	    return NULL;
> > +	  rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
> > +	  rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
> > +
> > +	  if (!rhs1_op0_type && !rhs1_op1_type)
> > +	    {
> > +	      rhs1_type = TREE_TYPE (rhs1_op0);
> > +	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
> 
> …here we should just be able to set rhs1_type, and leave vectype2 to the
> code below.
> 
> > +	    }
> > +	  else if (!rhs1_op0_type && rhs1_op1_type)
> > +	    {
> > +	      rhs1_type = TREE_TYPE (rhs1_op0);
> > +	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
> > +	      if (!vectype2)
> > +		return NULL;
> > +	      rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> > +						vectype2, stmt_vinfo);
> > +	    }
> > +	  else if (rhs1_op0_type && !rhs1_op1_type)
> > +	    {
> > +	      rhs1_type = TREE_TYPE (rhs1_op1);
> > +	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
> > +	      if (!vectype2)
> > +		return NULL;
> > +	      rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> > +						vectype2, stmt_vinfo);
> 
> Same for these two.
> 
> > +	    }
> > +	  else if (TYPE_PRECISION (rhs1_op0_type)
> > +		   != TYPE_PRECISION (rhs1_op1_type))
> > +	    {
> > +	      int tmp1 = (int)TYPE_PRECISION (rhs1_op0_type)
> > +			 - (int)TYPE_PRECISION (TREE_TYPE (lhs));
> > +	      int tmp2 = (int)TYPE_PRECISION (rhs1_op1_type)
> > +			 - (int)TYPE_PRECISION (TREE_TYPE (lhs));
> > +	      if ((tmp1 > 0 && tmp2 > 0)||(tmp1 < 0 && tmp2 < 0))
> 
> Minor formatting nit, sorry, but: GCC style is to put a space after
> (int) and on either side of ||.
> 
> Might be good to use the same numbering as the operands: tmp0 and tmp1
> instead of tmp1 and tmp2.
> 
> > +		{
> > +		  if (abs (tmp1) > abs (tmp2))
> > +		    {
> > +		      vectype2 = get_mask_type_for_scalar_type (vinfo,
> > +
> 	rhs1_op1_type);
> > +		      if (!vectype2)
> > +			return NULL;
> > +		      rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> > +							vectype2,
> stmt_vinfo);
> > +		    }
> > +		  else
> > +		    {
> > +		      vectype2 = get_mask_type_for_scalar_type (vinfo,
> > +
> 	rhs1_op0_type);
> > +		      if (!vectype2)
> > +			return NULL;
> > +		      rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> > +							vectype2,
> stmt_vinfo);
> > +		    }
> > +		  rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);
> 
> Here I think we can just go with rhs1_type = rhs1_op1_type if abs (tmp1) >
> abs (tmp2) (i.e. op1 is closer to the final type than op0) and rhs1_op0_type
> otherwise.
> 
> > +		}
> > +	      else
> > +		{
> > +		  rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> > +						    vectype1, stmt_vinfo);
> > +		  rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> > +						    vectype1, stmt_vinfo);
> > +		  rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);
> > +		  if (!rhs1_type)
> > +		    return NULL;
> > +		  vectype2 = get_mask_type_for_scalar_type (vinfo,
> rhs1_type);
> 
> and here I think rhs1_type can be:
> 
>   build_nonstandard_integer_type (TYPE_PRECISION (lhs_type), 1);
> 
> > +		}
> > +	    }
> > +	  else
> > +	    {
> > +	      rhs1_type = integer_type_for_mask (rhs1_op0, vinfo);
> > +	      if (!rhs1_type)
> > +		return NULL;
> > +	      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
> 
> Here either rhs1_op0_type or rhs1_op1_type should be OK.
> 
> > +	    }
> > +	  tmp = build2 (TREE_CODE (rhs1), TREE_TYPE (rhs1), rhs1_op0,
> rhs1_op1);
> > +	  rhs1 = tmp;
> >  	}
> >        else
> >  	return NULL;
> >
> > -      vectype2 = get_mask_type_for_scalar_type (vinfo, rhs1_type);
> > -
> > -      if (!vectype1 || !vectype2)
> > +      if (!vectype2)
> >  	return NULL;
> >
> >        /* Continue if a conversion is needed.  Also continue if we
> > have
> 
> With those changes, I think we would need to extend:
> 
>       /* Continue if a conversion is needed.  Also continue if we have
> 	 a comparison whose vector type would normally be different from
> 	 VECTYPE2 when considered in isolation.  In that case we'll
> 	 replace the comparison with an SSA name (so that we can record
> 	 its vector type) and behave as though the comparison was an SSA
> 	 name from the outset.  */
>       if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
> 		    TYPE_VECTOR_SUBPARTS (vectype2))
> 	  && (TREE_CODE (rhs1) == SSA_NAME
> 	      || rhs1_type == TREE_TYPE (TREE_OPERAND (rhs1, 0))))
> 
> …the condition on the last line so that it also checks whether rhs1_op0_type
> and rhs1_op1_type are null.  If they aren't null, we need to fall down to the
> code below.
> 
> 	return NULL;
> 
> And then here we create the build_mask_conversions to vectype2, like I
> think you did in the first patch, but reusing the rhs1_op0_type and
> rhs1_op1_type from above, and without changing vectype2:
> 
>       /* If rhs1 is a comparison we need to move it into a
> 	 separate statement.  */
>       if (TREE_CODE (rhs1) != SSA_NAME)
> 	{
> 	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
> 	  pattern_stmt = gimple_build_assign (tmp, rhs1);
> 	  rhs1 = tmp;
> 	  append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt,
> vectype2,
> 				  rhs1_type);
> 	}
> 
> Hope that works/makes sense.  Please let me know if it doesn't.
> 
> Thanks,
> Richard

Hi 

According to your suggestions,  I have modified the patch,  and it works.
Attached please find the v2 patch.
Bootstrap and tested on both aarch64 and x86 Linux platform, no new regression witnessed.
Ok for trunk?

Thanks 
Duan bo


[-- Attachment #2: pr96757-v2.patch --]
[-- Type: application/octet-stream, Size: 4782 bytes --]

From ea1905caf30b76e858dfe3883f53d8a7b80e9fae Mon Sep 17 00:00:00 2001
From: Duan bo <duanbo3@huawei.com>
Date: Tue, 29 Sep 2020 08:17:28 -0400
Subject: [PATCH] vect: Fix an ICE in vect_recog_mask_conversion_pattern

When processing the cond expression, vect_recog_mask_conversion_pattern
doesn't consider the situation that two operands of rhs1 are different
vectypes, leading to a vect ICE. This patch adds the identification and
handling of the situation to fix the problem.

gcc/ChangeLog:

	PR target/96757
	* tree-vect-patterns.c (vect_recog_mask_conversion_pattern): Add
	the identification and handling of the dropped situation in the
	cond expression processing phase.

gcc/testsuite/ChangeLog:

	PR target/96757
	* gcc.target/aarch64/pr96757.c: New test.
---
 gcc/testsuite/gcc.target/aarch64/pr96757.c | 23 ++++++++++
 gcc/tree-vect-patterns.c                   | 49 +++++++++++++++++++---
 2 files changed, 67 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr96757.c

diff --git a/gcc/testsuite/gcc.target/aarch64/pr96757.c b/gcc/testsuite/gcc.target/aarch64/pr96757.c
new file mode 100644
index 00000000000..122e39dca0e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr96757.c
@@ -0,0 +1,23 @@
+/* PR target/96757 */
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+short 
+fun1(short i, short j)
+{ 
+  return i * j; 
+}
+
+int 
+fun(int a, int b, int c) 
+{
+  int *v, z, k, m;
+  short f, d;
+  for (int i=0; i<c; i++) 
+  {
+    f= 4 <= d;
+    k= a > m;
+    z = f > k;
+    *v += fun1(z,b);
+  }
+}
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index db45740da3c..551f84820a9 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -4241,6 +4241,8 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
   tree lhs = NULL_TREE, rhs1, rhs2, tmp, rhs1_type, rhs2_type;
   tree vectype1, vectype2;
   stmt_vec_info pattern_stmt_info;
+  tree rhs1_op0 = NULL_TREE, rhs1_op1 = NULL_TREE;
+  tree rhs1_op0_type = NULL_TREE, rhs1_op1_type = NULL_TREE;
 
   /* Check for MASK_LOAD ans MASK_STORE calls requiring mask conversion.  */
   if (is_gimple_call (last_stmt)
@@ -4340,9 +4342,37 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
 
 	     it is better for b1 and b2 to use the mask type associated
 	     with int elements rather bool (byte) elements.  */
-	  rhs1_type = integer_type_for_mask (TREE_OPERAND (rhs1, 0), vinfo);
-	  if (!rhs1_type)
-	    rhs1_type = TREE_TYPE (TREE_OPERAND (rhs1, 0));
+	  rhs1_op0 = TREE_OPERAND (rhs1, 0);
+	  rhs1_op1 = TREE_OPERAND (rhs1, 1);
+	  if (!rhs1_op0 || !rhs1_op1)
+	    return NULL;
+	  rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
+	  rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
+
+	  if (!rhs1_op0_type)
+	    rhs1_type = TREE_TYPE (rhs1_op0);
+	  else if (!rhs1_op1_type)
+	    rhs1_type = TREE_TYPE (rhs1_op1);
+	  else if (TYPE_PRECISION (rhs1_op0_type)
+		   != TYPE_PRECISION (rhs1_op1_type))
+	    {
+	      int tmp0 = (int) TYPE_PRECISION (rhs1_op0_type)
+			 - (int) TYPE_PRECISION (TREE_TYPE (lhs));
+	      int tmp1 = (int) TYPE_PRECISION (rhs1_op1_type)
+			 - (int) TYPE_PRECISION (TREE_TYPE (lhs));
+	      if ((tmp0 > 0 && tmp1 > 0) || (tmp0 < 0 && tmp1 < 0))
+		{
+		  if (abs (tmp0) > abs (tmp1))
+		    rhs1_type = rhs1_op1_type;
+		  else
+		    rhs1_type = rhs1_op0_type;
+		}
+	      else
+		rhs1_type = build_nonstandard_integer_type
+		  (TYPE_PRECISION (TREE_TYPE (lhs)), 1);
+	    }
+	  else
+	    rhs1_type = rhs1_op0_type;
 	}
       else
 	return NULL;
@@ -4361,7 +4391,7 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
       if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
 		    TYPE_VECTOR_SUBPARTS (vectype2))
 	  && (TREE_CODE (rhs1) == SSA_NAME
-	      || rhs1_type == TREE_TYPE (TREE_OPERAND (rhs1, 0))))
+	      || !rhs1_op0_type || !rhs1_op1_type))
 	return NULL;
 
       /* If rhs1 is invariant and we can promote it leave the COND_EXPR
@@ -4393,7 +4423,16 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
       if (TREE_CODE (rhs1) != SSA_NAME)
 	{
 	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
-	  pattern_stmt = gimple_build_assign (tmp, rhs1);
+	  if (rhs1_op0_type && TYPE_PRECISION (rhs1_op0_type)
+				!= TYPE_PRECISION (rhs1_type))
+	    rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
+					      vectype2, stmt_vinfo);
+	  if (rhs1_op1_type && TYPE_PRECISION (rhs1_op1_type)
+				!= TYPE_PRECISION (rhs1_type))
+	    rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
+					      vectype2, stmt_vinfo);
+	  pattern_stmt = gimple_build_assign (tmp, TREE_CODE (rhs1),
+			 		      rhs1_op0, rhs1_op1);
 	  rhs1 = tmp;
 	  append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt, vectype2,
 				  rhs1_type);
-- 
2.19.1

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

[Richard Sandiford]

Thanks for the update, looks good apart from…

"duanbo (C)" <duanbo3@huawei.com> writes:
> @@ -4361,7 +4391,7 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
>        if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
>  		    TYPE_VECTOR_SUBPARTS (vectype2))
>  	  && (TREE_CODE (rhs1) == SSA_NAME
> -	      || rhs1_type == TREE_TYPE (TREE_OPERAND (rhs1, 0))))
> +	      || !rhs1_op0_type || !rhs1_op1_type))
>  	return NULL;

…I think this should be:

	  && (TREE_CODE (rhs1) == SSA_NAME
	      || (!rhs1_op0_type && !rhs1_op1_type))

i.e. punt only if both types are already OK.  If one operand wants
a specific mask type, we should continue to the code below and attach
the chosen type to the comparison.

Although I guess this simplifies to:

      if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
                    TYPE_VECTOR_SUBPARTS (vectype2))
          && !rhs1_op0_type
          && !rhs1_op1_type)
        return NULL;

(I think the comment above the code is still accurate with this change.)

> @@ -4393,7 +4423,16 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
>        if (TREE_CODE (rhs1) != SSA_NAME)
>  	{
>  	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
> -	  pattern_stmt = gimple_build_assign (tmp, rhs1);
> +	  if (rhs1_op0_type && TYPE_PRECISION (rhs1_op0_type)
> +				!= TYPE_PRECISION (rhs1_type))
> +	    rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> +					      vectype2, stmt_vinfo);
> +	  if (rhs1_op1_type && TYPE_PRECISION (rhs1_op1_type)
> +				!= TYPE_PRECISION (rhs1_type))

Very minor -- I would have fixed this up before committing if it
wasn't for the above -- but: GCC formatting is instead:

	  if (rhs1_op1_type
	      && TYPE_PRECISION (rhs1_op1_type) != TYPE_PRECISION (rhs1_type))

LGTM with those changes, thanks.

Richard

> +	    rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> +					      vectype2, stmt_vinfo);
> +	  pattern_stmt = gimple_build_assign (tmp, TREE_CODE (rhs1),
> +			 		      rhs1_op0, rhs1_op1);
>  	  rhs1 = tmp;
>  	  append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt, vectype2,
>  				  rhs1_type);

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

[duanbo (C)]

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



> -----Original Message-----
> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
> Sent: Wednesday, September 30, 2020 6:38 PM
> To: duanbo (C) <duanbo3@huawei.com>
> Cc: GCC Patches <gcc-patches@gcc.gnu.org>
> Subject: Re: [PATCH PR96757] aarch64: ICE during GIMPLE pass: vect
> 
> Thanks for the update, looks good apart from…
> 
> "duanbo (C)" <duanbo3@huawei.com> writes:
> > @@ -4361,7 +4391,7 @@ vect_recog_mask_conversion_pattern (vec_info
> *vinfo,
> >        if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
> >  		    TYPE_VECTOR_SUBPARTS (vectype2))
> >  	  && (TREE_CODE (rhs1) == SSA_NAME
> > -	      || rhs1_type == TREE_TYPE (TREE_OPERAND (rhs1, 0))))
> > +	      || !rhs1_op0_type || !rhs1_op1_type))
> >  	return NULL;
> 
> …I think this should be:
> 
> 	  && (TREE_CODE (rhs1) == SSA_NAME
> 	      || (!rhs1_op0_type && !rhs1_op1_type))
> 
> i.e. punt only if both types are already OK.  If one operand wants a specific
> mask type, we should continue to the code below and attach the chosen
> type to the comparison.
> 
> Although I guess this simplifies to:
> 
>       if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
>                     TYPE_VECTOR_SUBPARTS (vectype2))
>           && !rhs1_op0_type
>           && !rhs1_op1_type)
>         return NULL;
> 
> (I think the comment above the code is still accurate with this change.)
> 
> > @@ -4393,7 +4423,16 @@ vect_recog_mask_conversion_pattern
> (vec_info *vinfo,
> >        if (TREE_CODE (rhs1) != SSA_NAME)
> >  	{
> >  	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
> > -	  pattern_stmt = gimple_build_assign (tmp, rhs1);
> > +	  if (rhs1_op0_type && TYPE_PRECISION (rhs1_op0_type)
> > +				!= TYPE_PRECISION (rhs1_type))
> > +	    rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> > +					      vectype2, stmt_vinfo);
> > +	  if (rhs1_op1_type && TYPE_PRECISION (rhs1_op1_type)
> > +				!= TYPE_PRECISION (rhs1_type))
> 
> Very minor -- I would have fixed this up before committing if it wasn't for the
> above -- but: GCC formatting is instead:
> 
> 	  if (rhs1_op1_type
> 	      && TYPE_PRECISION (rhs1_op1_type) != TYPE_PRECISION
> (rhs1_type))
> 
> LGTM with those changes, thanks.
> 
> Richard
> 
> > +	    rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> > +					      vectype2, stmt_vinfo);
> > +	  pattern_stmt = gimple_build_assign (tmp, TREE_CODE (rhs1),
> > +			 		      rhs1_op0, rhs1_op1);
> >  	  rhs1 = tmp;
> >  	  append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt,
> vectype2,
> >  				  rhs1_type);

Sorry for the late reply.
I have modified the patch according to your suggestion, and it works well.
Ok for trunk?

Thanks,
Duan bo


[-- Attachment #2: pr96757-v3.patch --]
[-- Type: application/octet-stream, Size: 4814 bytes --]

From 3713b21e6b0fb698ea8e579d1fcfe9623e281f2c Mon Sep 17 00:00:00 2001
From: Duan bo <duanbo3@huawei.com>
Date: Fri, 9 Oct 2020 02:17:13 -0400
Subject: [PATCH] vect: Fix an ICE in vect_recog_mask_conversion_pattern

When processing the cond expression, vect_recog_mask_conversion_pattern
doesn't consider the situation that two operands of rhs1 are different
vectypes, leading to a vect ICE. This patch adds the identification and
handling of the situation to fix the problem.

gcc/ChangeLog:

	PR target/96757
	* tree-vect-patterns.c (vect_recog_mask_conversion_pattern): Add
	the identification and handling of the dropped situation in the
	cond expression processing phase.

gcc/testsuite/ChangeLog:

	PR target/96757
	* gcc.target/aarch64/pr96757.c: New test.
---
 gcc/testsuite/gcc.target/aarch64/pr96757.c | 23 ++++++++++
 gcc/tree-vect-patterns.c                   | 51 +++++++++++++++++++---
 2 files changed, 68 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr96757.c

diff --git a/gcc/testsuite/gcc.target/aarch64/pr96757.c b/gcc/testsuite/gcc.target/aarch64/pr96757.c
new file mode 100644
index 00000000000..122e39dca0e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr96757.c
@@ -0,0 +1,23 @@
+/* PR target/96757 */
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+short 
+fun1(short i, short j)
+{ 
+  return i * j; 
+}
+
+int 
+fun(int a, int b, int c) 
+{
+  int *v, z, k, m;
+  short f, d;
+  for (int i=0; i<c; i++) 
+  {
+    f= 4 <= d;
+    k= a > m;
+    z = f > k;
+    *v += fun1(z,b);
+  }
+}
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 71e4e106202..43e43796490 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -4245,6 +4245,8 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
   tree lhs = NULL_TREE, rhs1, rhs2, tmp, rhs1_type, rhs2_type;
   tree vectype1, vectype2;
   stmt_vec_info pattern_stmt_info;
+  tree rhs1_op0 = NULL_TREE, rhs1_op1 = NULL_TREE;
+  tree rhs1_op0_type = NULL_TREE, rhs1_op1_type = NULL_TREE;
 
   /* Check for MASK_LOAD ans MASK_STORE calls requiring mask conversion.  */
   if (is_gimple_call (last_stmt)
@@ -4344,9 +4346,37 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
 
 	     it is better for b1 and b2 to use the mask type associated
 	     with int elements rather bool (byte) elements.  */
-	  rhs1_type = integer_type_for_mask (TREE_OPERAND (rhs1, 0), vinfo);
-	  if (!rhs1_type)
-	    rhs1_type = TREE_TYPE (TREE_OPERAND (rhs1, 0));
+	  rhs1_op0 = TREE_OPERAND (rhs1, 0);
+	  rhs1_op1 = TREE_OPERAND (rhs1, 1);
+	  if (!rhs1_op0 || !rhs1_op1)
+	    return NULL;
+	  rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
+	  rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
+
+	  if (!rhs1_op0_type)
+	    rhs1_type = TREE_TYPE (rhs1_op0);
+	  else if (!rhs1_op1_type)
+	    rhs1_type = TREE_TYPE (rhs1_op1);
+	  else if (TYPE_PRECISION (rhs1_op0_type)
+		   != TYPE_PRECISION (rhs1_op1_type))
+	    {
+	      int tmp0 = (int) TYPE_PRECISION (rhs1_op0_type)
+			 - (int) TYPE_PRECISION (TREE_TYPE (lhs));
+	      int tmp1 = (int) TYPE_PRECISION (rhs1_op1_type)
+			 - (int) TYPE_PRECISION (TREE_TYPE (lhs));
+	      if ((tmp0 > 0 && tmp1 > 0) || (tmp0 < 0 && tmp1 < 0))
+		{
+		  if (abs (tmp0) > abs (tmp1))
+		    rhs1_type = rhs1_op1_type;
+		  else
+		    rhs1_type = rhs1_op0_type;
+		}
+	      else
+		rhs1_type = build_nonstandard_integer_type
+		  (TYPE_PRECISION (TREE_TYPE (lhs)), 1);
+	    }
+	  else
+	    rhs1_type = rhs1_op0_type;
 	}
       else
 	return NULL;
@@ -4364,8 +4394,8 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
 	 name from the outset.  */
       if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
 		    TYPE_VECTOR_SUBPARTS (vectype2))
-	  && (TREE_CODE (rhs1) == SSA_NAME
-	      || rhs1_type == TREE_TYPE (TREE_OPERAND (rhs1, 0))))
+	  && !rhs1_op0_type
+	  && !rhs1_op1_type)
 	return NULL;
 
       /* If rhs1 is invariant and we can promote it leave the COND_EXPR
@@ -4397,7 +4427,16 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
       if (TREE_CODE (rhs1) != SSA_NAME)
 	{
 	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
-	  pattern_stmt = gimple_build_assign (tmp, rhs1);
+	  if (rhs1_op0_type
+	      && TYPE_PRECISION (rhs1_op0_type) != TYPE_PRECISION (rhs1_type))
+	    rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
+					      vectype2, stmt_vinfo);
+	  if (rhs1_op1_type
+	      && TYPE_PRECISION (rhs1_op1_type) != TYPE_PRECISION (rhs1_type))
+	    rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
+				      vectype2, stmt_vinfo);
+	  pattern_stmt = gimple_build_assign (tmp, TREE_CODE (rhs1),
+			 		      rhs1_op0, rhs1_op1);
 	  rhs1 = tmp;
 	  append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt, vectype2,
 				  rhs1_type);
-- 
2.19.1

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

[Richard Sandiford]

"duanbo (C)" <duanbo3@huawei.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford [mailto:richard.sandiford@arm.com]
>> Sent: Wednesday, September 30, 2020 6:38 PM
>> To: duanbo (C) <duanbo3@huawei.com>
>> Cc: GCC Patches <gcc-patches@gcc.gnu.org>
>> Subject: Re: [PATCH PR96757] aarch64: ICE during GIMPLE pass: vect
>> 
>> Thanks for the update, looks good apart from…
>> 
>> "duanbo (C)" <duanbo3@huawei.com> writes:
>> > @@ -4361,7 +4391,7 @@ vect_recog_mask_conversion_pattern (vec_info
>> *vinfo,
>> >        if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
>> >  		    TYPE_VECTOR_SUBPARTS (vectype2))
>> >  	  && (TREE_CODE (rhs1) == SSA_NAME
>> > -	      || rhs1_type == TREE_TYPE (TREE_OPERAND (rhs1, 0))))
>> > +	      || !rhs1_op0_type || !rhs1_op1_type))
>> >  	return NULL;
>> 
>> …I think this should be:
>> 
>> 	  && (TREE_CODE (rhs1) == SSA_NAME
>> 	      || (!rhs1_op0_type && !rhs1_op1_type))
>> 
>> i.e. punt only if both types are already OK.  If one operand wants a specific
>> mask type, we should continue to the code below and attach the chosen
>> type to the comparison.
>> 
>> Although I guess this simplifies to:
>> 
>>       if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1),
>>                     TYPE_VECTOR_SUBPARTS (vectype2))
>>           && !rhs1_op0_type
>>           && !rhs1_op1_type)
>>         return NULL;
>> 
>> (I think the comment above the code is still accurate with this change.)
>> 
>> > @@ -4393,7 +4423,16 @@ vect_recog_mask_conversion_pattern
>> (vec_info *vinfo,
>> >        if (TREE_CODE (rhs1) != SSA_NAME)
>> >  	{
>> >  	  tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
>> > -	  pattern_stmt = gimple_build_assign (tmp, rhs1);
>> > +	  if (rhs1_op0_type && TYPE_PRECISION (rhs1_op0_type)
>> > +				!= TYPE_PRECISION (rhs1_type))
>> > +	    rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
>> > +					      vectype2, stmt_vinfo);
>> > +	  if (rhs1_op1_type && TYPE_PRECISION (rhs1_op1_type)
>> > +				!= TYPE_PRECISION (rhs1_type))
>> 
>> Very minor -- I would have fixed this up before committing if it wasn't for the
>> above -- but: GCC formatting is instead:
>> 
>> 	  if (rhs1_op1_type
>> 	      && TYPE_PRECISION (rhs1_op1_type) != TYPE_PRECISION
>> (rhs1_type))
>> 
>> LGTM with those changes, thanks.
>> 
>> Richard
>> 
>> > +	    rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
>> > +					      vectype2, stmt_vinfo);
>> > +	  pattern_stmt = gimple_build_assign (tmp, TREE_CODE (rhs1),
>> > +			 		      rhs1_op0, rhs1_op1);
>> >  	  rhs1 = tmp;
>> >  	  append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt,
>> vectype2,
>> >  				  rhs1_type);
>
> Sorry for the late reply.
> I have modified the patch according to your suggestion, and it works well.

Looks good, thanks.  Pushed to trunk.

Richard