From: "duanbo (C)" <duanbo3@huawei.com>
To: Richard Sandiford <richard.sandiford@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: RE: [PATCH PR96757] aarch64: ICE during GIMPLE pass: vect
Date: Wed, 30 Sep 2020 02:03:43 +0000 [thread overview]
Message-ID: <d878339c5e3e4bafab4cf1843d1ae243@huawei.com> (raw)
In-Reply-To: <mptsgb7crsv.fsf@arm.com>
[-- 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
next prev parent reply other threads:[~2020-09-30 2:03 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-26 9:30 duanbo (C)
2020-08-28 18:30 ` Richard Sandiford
2020-09-19 8:06 ` duanbo (C)
2020-09-24 11:55 ` Richard Sandiford
2020-09-30 2:03 ` duanbo (C) [this message]
2020-09-30 10:38 ` Richard Sandiford
2020-10-09 7:21 ` duanbo (C)
2020-10-12 11:33 ` Richard Sandiford
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d878339c5e3e4bafab4cf1843d1ae243@huawei.com \
--to=duanbo3@huawei.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=richard.sandiford@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).