From: Richard Sandiford <richard.sandiford@arm.com>
To: Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>
Cc: Richard Biener <richard.guenther@gmail.com>,
gcc Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [SVE] PR86753
Date: Mon, 09 Sep 2019 11:15:00 -0000 [thread overview]
Message-ID: <mpth85laffc.fsf@arm.com> (raw)
In-Reply-To: <CAAgBjM=BNtAkcj__EhBWqd5FYZgQJiMO_gR4aq49ixdE=_aAJQ@mail.gmail.com> (Prathamesh Kulkarni's message of "Thu, 5 Sep 2019 18:20:29 +0530")
Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> With patch, the only following FAIL remains for aarch64-sve.exp:
> FAIL: gcc.target/aarch64/sve/cond_unary_2.c -march=armv8.2-a+sve
> scan-assembler-times \\tmovprfx\\t 6
> which now contains 14.
> Should I adjust the test, assuming the change isn't a regression ?
Well, it is kind-of a regression, but it really just means that the
integer code is now consistent with the floating-point code in having
an unnecessary MOVPRFX. So I think adjusting the count is fine.
Presumably any future fix for the existing redundant MOVPRFXs will
apply to the new ones as well.
The patch looks good to me, just some very minor nits:
> @@ -8309,11 +8309,12 @@ vect_double_mask_nunits (tree type)
>
> /* Record that a fully-masked version of LOOP_VINFO would need MASKS to
> contain a sequence of NVECTORS masks that each control a vector of type
> - VECTYPE. */
> + VECTYPE. SCALAR_MASK if non-null, represents the mask used for corresponding
> + load/store stmt. */
Should be two spaces between sentences. Maybe:
VECTYPE. If SCALAR_MASK is nonnull, the fully-masked loop would AND
these vector masks with the vector version of SCALAR_MASK. */
since the mask isn't necessarily for a load or store statement.
> [...]
> @@ -1879,7 +1879,8 @@ static tree permute_vec_elements (tree, tree, tree, stmt_vec_info,
> says how the load or store is going to be implemented and GROUP_SIZE
> is the number of load or store statements in the containing group.
> If the access is a gather load or scatter store, GS_INFO describes
> - its arguments.
> + its arguments. SCALAR_MASK is the scalar mask used for corresponding
> + load or store stmt.
Maybe:
its arguments. If the load or store is conditional, SCALAR_MASK is the
condition under which it occurs.
since SCALAR_MASK can be null here too.
> [...]
> @@ -9975,6 +9978,31 @@ vectorizable_condition (stmt_vec_info stmt_info, gimple_stmt_iterator *gsi,
> /* Handle cond expr. */
> for (j = 0; j < ncopies; j++)
> {
> + tree loop_mask = NULL_TREE;
> + bool swap_cond_operands = false;
> +
> + if (loop_vinfo && LOOP_VINFO_FULLY_MASKED_P (loop_vinfo))
> + {
> + scalar_cond_masked_key cond (cond_expr, ncopies);
> + if (loop_vinfo->scalar_cond_masked_set.contains (cond))
> + {
> + vec_loop_masks *masks = &LOOP_VINFO_MASKS (loop_vinfo);
> + loop_mask = vect_get_loop_mask (gsi, masks, ncopies, vectype, j);
> + }
> + else
> + {
> + cond.code = invert_tree_comparison (cond.code,
> + HONOR_NANS (TREE_TYPE (cond.op0)));
Long line. Maybe just split it out into a separate assignment:
bool honor_nans = HONOR_NANS (TREE_TYPE (cond.op0));
cond.code = invert_tree_comparison (cond.code, honor_nans);
> + if (loop_vinfo->scalar_cond_masked_set.contains (cond))
> + {
> + vec_loop_masks *masks = &LOOP_VINFO_MASKS (loop_vinfo);
> + loop_mask = vect_get_loop_mask (gsi, masks, ncopies, vectype, j);
Long line here too.
> [...]
> @@ -10090,6 +10121,26 @@ vectorizable_condition (stmt_vec_info stmt_info, gimple_stmt_iterator *gsi,
> }
> }
> }
> +
> + if (loop_mask)
> + {
> + if (COMPARISON_CLASS_P (vec_compare))
> + {
> + tree tmp = make_ssa_name (vec_cmp_type);
> + gassign *g = gimple_build_assign (tmp,
> + TREE_CODE (vec_compare),
> + TREE_OPERAND (vec_compare, 0),
d> + TREE_OPERAND (vec_compare, 1));
Two long lines.
> + vect_finish_stmt_generation (stmt_info, g, gsi);
> + vec_compare = tmp;
> + }
> +
> + tree tmp2 = make_ssa_name (vec_cmp_type);
> + gassign *g = gimple_build_assign (tmp2, BIT_AND_EXPR, vec_compare, loop_mask);
Long line here too.
> [...]
> diff --git a/gcc/tree-vectorizer.c b/gcc/tree-vectorizer.c
> index dc181524744..c4b2d8e8647 100644
> --- a/gcc/tree-vectorizer.c
> +++ b/gcc/tree-vectorizer.c
> @@ -1513,3 +1513,39 @@ make_pass_ipa_increase_alignment (gcc::context *ctxt)
> {
> return new pass_ipa_increase_alignment (ctxt);
> }
> +
> +/* If code(T) is comparison op or def of comparison stmt,
> + extract it's operands.
> + Else return <NE_EXPR, T, 0>. */
> +
> +void
> +scalar_cond_masked_key::get_cond_ops_from_tree (tree t)
> +{
> + if (TREE_CODE_CLASS (TREE_CODE (t)) == tcc_comparison)
> + {
> + this->code = TREE_CODE (t);
> + this->op0 = TREE_OPERAND (t, 0);
> + this->op1 = TREE_OPERAND (t, 1);
> + return;
> + }
> +
> + if (TREE_CODE (t) == SSA_NAME)
> + {
> + gassign *stmt = dyn_cast<gassign *> (SSA_NAME_DEF_STMT (t));
> + if (stmt)
> + {
Might as well do this as:
if (TREE_CODE (t) == SSA_NAME)
if (gassign *stmt = dyn_cast<gassign *> (SSA_NAME_DEF_STMT (t)))
{
The patch (as hoped) introduces some XPASSes:
XPASS: gcc.target/aarch64/sve/cond_cnot_2.c scan-assembler-not \\tsel\\t
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmge\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, #0\\.0\\n 21
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmge\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, z[0-9]+\\.d\\n 42
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmge\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, #0\\.0\\n 15
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmge\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, z[0-9]+\\.s\\n 30
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmgt\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, #0\\.0\\n 21
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmgt\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, z[0-9]+\\.d\\n 42
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmgt\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, #0\\.0\\n 15
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmgt\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, z[0-9]+\\.s\\n 30
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmle\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, #0\\.0\\n 21
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmle\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, z[0-9]+\\.d\\n 42
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmle\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, #0\\.0\\n 15
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmle\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, z[0-9]+\\.s\\n 30
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmlt\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, #0\\.0\\n 21
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmlt\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, z[0-9]+\\.d\\n 42
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmlt\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, #0\\.0\\n 15
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmlt\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, z[0-9]+\\.s\\n 30
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmuo\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, z[0-9]+\\.d\\n 252
XPASS: gcc.target/aarch64/sve/vcond_4.c scan-assembler-times \\tfcmuo\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, z[0-9]+\\.s\\n 180
XPASS: gcc.target/aarch64/sve/vcond_5.c scan-assembler-times \\tfcmge\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, #0\\.0 21
XPASS: gcc.target/aarch64/sve/vcond_5.c scan-assembler-times \\tfcmge\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, z[0-9]+\\.d 42
XPASS: gcc.target/aarch64/sve/vcond_5.c scan-assembler-times \\tfcmge\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, #0\\.0 15
XPASS: gcc.target/aarch64/sve/vcond_5.c scan-assembler-times \\tfcmge\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, z[0-9]+\\.s 30
XPASS: gcc.target/aarch64/sve/vcond_5.c scan-assembler-times \\tfcmle\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, #0\\.0 21
XPASS: gcc.target/aarch64/sve/vcond_5.c scan-assembler-times \\tfcmle\\tp[0-9]+\\.d, p[0-7]/z, z[0-9]+\\.d, z[0-9]+\\.d 42
XPASS: gcc.target/aarch64/sve/vcond_5.c scan-assembler-times \\tfcmle\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, #0\\.0 15
XPASS: gcc.target/aarch64/sve/vcond_5.c scan-assembler-times \\tfcmle\\tp[0-9]+\\.s, p[0-7]/z, z[0-9]+\\.s, z[0-9]+\\.s 30
Could you remove the associated xfails (and comments above them where
appropriate)?
OK with those changes from my POV, but please give Richi a day or so
to object.
Thanks for doing this.
Richard
next prev parent reply other threads:[~2019-09-09 11:15 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-14 15:53 Prathamesh Kulkarni
2019-08-14 16:59 ` Richard Biener
2019-08-14 17:01 ` Richard Biener
2019-08-14 21:22 ` Richard Sandiford
2019-08-21 20:10 ` Prathamesh Kulkarni
2019-08-22 12:05 ` Richard Biener
2019-08-23 12:46 ` Prathamesh Kulkarni
2019-08-23 13:47 ` Richard Sandiford
2019-08-23 14:30 ` Prathamesh Kulkarni
2019-08-23 14:34 ` Richard Sandiford
2019-08-26 5:59 ` Prathamesh Kulkarni
2019-08-26 11:46 ` Richard Biener
2019-08-26 13:39 ` Prathamesh Kulkarni
2019-08-27 10:41 ` Richard Sandiford
2019-08-27 11:31 ` Richard Biener
2019-08-27 12:52 ` Richard Sandiford
2019-08-27 15:55 ` Prathamesh Kulkarni
2019-08-27 17:39 ` Richard Sandiford
2019-08-27 20:10 ` Prathamesh Kulkarni
2019-08-28 9:42 ` Richard Sandiford
2019-08-30 12:09 ` Richard Biener
2019-08-31 16:56 ` Prathamesh Kulkarni
2019-09-05 9:00 ` Richard Sandiford
2019-09-05 12:51 ` Prathamesh Kulkarni
2019-09-09 11:15 ` Richard Sandiford [this message]
2019-09-09 16:37 ` Prathamesh Kulkarni
2019-09-09 20:56 ` Prathamesh Kulkarni
2019-09-10 12:20 ` Richard Sandiford
2019-09-10 13:35 ` Matthew Malcomson
2019-09-10 21:36 ` Prathamesh Kulkarni
2019-09-16 15:54 ` Prathamesh Kulkarni
2019-09-25 16:18 ` Prathamesh Kulkarni
2019-10-02 23:42 ` Prathamesh Kulkarni
2019-10-04 10:38 ` Richard Biener
2019-10-08 0:10 ` Prathamesh Kulkarni
2019-10-08 7:51 ` Richard Sandiford
2019-10-09 3:23 ` Prathamesh Kulkarni
2019-10-15 6:11 ` Prathamesh Kulkarni
2019-10-15 11:40 ` Richard Biener
2019-10-16 12:13 ` Richard Sandiford
2019-10-18 5:20 ` Prathamesh Kulkarni
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=mpth85laffc.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=prathamesh.kulkarni@linaro.org \
--cc=richard.guenther@gmail.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).