From: James Greenhalgh <james.greenhalgh@arm.com>
To: Michael Collison <Michael.Collison@arm.com>
Cc: Richard Sandiford <richard.sandiford@linaro.org>,
Richard Biener <richard.guenther@gmail.com>,
Richard Kenner <kenner@vlsi1.ultra.nyu.edu>,
GCC Patches <gcc-patches@gcc.gnu.org>, nd <nd@arm.com>,
Andrew Pinski <pinskia@gmail.com>
Subject: Re: [PATCH] [Aarch64] Optimize subtract in shift counts
Date: Fri, 29 Sep 2017 23:01:00 -0000 [thread overview]
Message-ID: <20170929230123.GB22958@arm.com> (raw)
In-Reply-To: <HE1PR0802MB2377C7416786659EC024D4FE956C0@HE1PR0802MB2377.eurprd08.prod.outlook.com>
On Fri, Sep 15, 2017 at 09:15:17AM +0100, Michael Collison wrote:
> Patch updated with comments to simplify pattern .from Richard Sandiford. Okay for trunk?
OK.
Thanks,
James
>
> 2017-09-14 Michael Collison <michael.collison@arm.com>
>
> * config/aarch64/aarch64.md (*aarch64_reg_<optab>_minus<mode>3):
> New pattern.
>
> 2017-09-14 Michael Collison <michael.collison@arm.com>
>
> * gcc.target/aarch64/var_shift_mask_2.c: New test.
>
> Richard Sandiford <richard.sandiford@linaro.org> writes:
> > Richard Sandiford <richard.sandiford@linaro.org> writes:
> >> Michael Collison <Michael.Collison@arm.com> writes:
> >>> Richard,
> >>>
> >>> The problem with this approach for Aarch64 is that
> >>> TARGET_SHIFT_TRUNCATION_MASK is based on SHIFT_COUNT_TRUNCATED which
> >>> is normally 0 as it based on the TARGET_SIMD flag.
> >>
> >> Maybe I'm wrong, but that seems like a missed optimisation in itself.
>
> Sorry to follow up on myself yet again, but I'd forgotten this was because we allow the SIMD unit to do scalar shifts. So I guess we have no choice, even though it seems unfortunate.
>
> > +(define_insn_and_split "*aarch64_reg_<optab>_minus<mode>3"
> > + [(set (match_operand:GPI 0 "register_operand" "=&r")
> > + (ASHIFT:GPI
> > + (match_operand:GPI 1 "register_operand" "r")
> > + (minus:QI (match_operand 2 "const_int_operand" "n")
> > + (match_operand:QI 3 "register_operand" "r"))))]
> > + "INTVAL (operands[2]) == GET_MODE_BITSIZE (<MODE>mode)"
> > + "#"
> > + "&& true"
> > + [(const_int 0)]
> > + {
> > + /* Handle cases where operand 3 is a plain QI register, or
> > + a subreg with either a SImode or DImode register. */
> > +
> > + rtx subreg_tmp = (REG_P (operands[3])
> > + ? gen_lowpart_SUBREG (SImode, operands[3])
> > + : SUBREG_REG (operands[3]));
> > +
> > + if (REG_P (subreg_tmp) && GET_MODE (subreg_tmp) == DImode)
> > + subreg_tmp = gen_lowpart_SUBREG (SImode, subreg_tmp);
>
> I think this all simplifies to:
>
> rtx subreg_tmp = gen_lowpart (SImode, operands[3]);
>
> (or it would be worth having a comment that explains why not).
> As well as being shorter, it will properly simplify hard REGs to new hard REGs.
>
> > + rtx tmp = (can_create_pseudo_p () ? gen_reg_rtx (SImode)
> > + : operands[0]);
> > +
> > + if (<MODE>mode == DImode && !can_create_pseudo_p ())
> > + tmp = gen_lowpart_SUBREG (SImode, operands[0]);
>
> I think this too would be simpler with gen_lowpart:
>
> rtx tmp = (can_create_pseudo_p () ? gen_reg_rtx (SImode)
> : gen_lowpart (SImode, operands[0]));
>
> > +
> > + emit_insn (gen_negsi2 (tmp, subreg_tmp));
> > +
> > + rtx and_op = gen_rtx_AND (SImode, tmp,
> > + GEN_INT (GET_MODE_BITSIZE (<MODE>mode) - 1));
> > +
> > + rtx subreg_tmp2 = gen_lowpart_SUBREG (QImode, and_op);
> > +
> > + emit_insn (gen_<optab><mode>3 (operands[0], operands[1], subreg_tmp2));
> > + DONE;
> > + }
> > +)
>
> The pattern should probably set the "length" attribute to 8.
>
> Looks good to me with those changes FWIW.
>
> Thanks,
> Richard
next prev parent reply other threads:[~2017-09-29 23:01 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-07 20:36 Michael Collison
2017-08-07 20:44 ` Andrew Pinski
2017-08-07 21:02 ` Richard Kenner
2017-08-07 23:44 ` Michael Collison
2017-08-08 1:56 ` Richard Kenner
2017-08-08 4:06 ` Michael Collison
2017-08-08 12:13 ` Richard Kenner
2017-08-08 19:46 ` Michael Collison
2017-08-08 19:52 ` Richard Kenner
2017-08-08 19:59 ` Michael Collison
2017-08-08 20:04 ` Richard Kenner
2017-08-08 20:18 ` Michael Collison
2017-08-08 20:20 ` Richard Kenner
2017-08-08 20:33 ` Michael Collison
2017-08-14 8:58 ` Richard Biener
2017-08-15 7:28 ` Michael Collison
2017-08-21 19:11 ` Richard Sandiford
2017-08-21 19:14 ` Richard Biener
2017-08-22 8:17 ` Richard Sandiford
2017-08-22 8:46 ` Richard Biener
2017-08-22 8:50 ` Richard Sandiford
2017-09-06 9:11 ` Michael Collison
2017-09-06 10:32 ` Richard Sandiford
2017-09-06 15:41 ` Michael Collison
2017-09-06 16:53 ` Richard Sandiford
2017-09-06 16:56 ` Richard Sandiford
2017-09-06 17:21 ` Richard Sandiford
2017-09-15 8:15 ` Michael Collison
2017-09-29 23:01 ` James Greenhalgh [this message]
2017-08-07 20:58 ` Richard Kenner
2017-08-08 10:04 Wilco Dijkstra
2017-08-22 11:01 Wilco Dijkstra
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=20170929230123.GB22958@arm.com \
--to=james.greenhalgh@arm.com \
--cc=Michael.Collison@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kenner@vlsi1.ultra.nyu.edu \
--cc=nd@arm.com \
--cc=pinskia@gmail.com \
--cc=richard.guenther@gmail.com \
--cc=richard.sandiford@linaro.org \
/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).