public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Richard Sandiford <richard.sandiford@arm.com>
To: Tamar Christina <Tamar.Christina@arm.com>
Cc: "gcc-patches\@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
	 Richard Earnshaw <Richard.Earnshaw@arm.com>,  nd <nd@arm.com>,
	 Marcus Shawcroft <Marcus.Shawcroft@arm.com>
Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab.
Date: Tue, 15 Nov 2022 11:33:34 +0000	[thread overview]
Message-ID: <mptiljgjvu9.fsf@arm.com> (raw)
In-Reply-To: <VI1PR08MB53253860DD16B5844B9DCEF3FF049@VI1PR08MB5325.eurprd08.prod.outlook.com> (Tamar Christina's message of "Tue, 15 Nov 2022 11:23:24 +0000")

Tamar Christina <Tamar.Christina@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandiford@arm.com>
>> Sent: Tuesday, November 15, 2022 11:15 AM
>> To: Tamar Christina <Tamar.Christina@arm.com>
>> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw
>> <Richard.Earnshaw@arm.com>; nd <nd@arm.com>; Marcus Shawcroft
>> <Marcus.Shawcroft@arm.com>
>> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab.
>> 
>> Tamar Christina <Tamar.Christina@arm.com> writes:
>> >> -----Original Message-----
>> >> From: Richard Sandiford <richard.sandiford@arm.com>
>> >> Sent: Tuesday, November 15, 2022 10:51 AM
>> >> To: Tamar Christina <Tamar.Christina@arm.com>
>> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw
>> >> <Richard.Earnshaw@arm.com>; nd <nd@arm.com>; Marcus Shawcroft
>> >> <Marcus.Shawcroft@arm.com>
>> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab.
>> >>
>> >> Tamar Christina <Tamar.Christina@arm.com> writes:
>> >> >> -----Original Message-----
>> >> >> From: Richard Sandiford <richard.sandiford@arm.com>
>> >> >> Sent: Tuesday, November 15, 2022 10:36 AM
>> >> >> To: Tamar Christina <Tamar.Christina@arm.com>
>> >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw
>> >> >> <Richard.Earnshaw@arm.com>; nd <nd@arm.com>; Marcus
>> Shawcroft
>> >> >> <Marcus.Shawcroft@arm.com>
>> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab.
>> >> >>
>> >> >> Tamar Christina <Tamar.Christina@arm.com> writes:
>> >> >> > Hello,
>> >> >> >
>> >> >> > Ping and updated patch.
>> >> >> >
>> >> >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>> >> >> >
>> >> >> > Ok for master?
>> >> >> >
>> >> >> > Thanks,
>> >> >> > Tamar
>> >> >> >
>> >> >> > gcc/ChangeLog:
>> >> >> >
>> >> >> >         * config/aarch64/aarch64.md (*tb<optab><mode>1): Rename
>> to...
>> >> >> >         (*tb<optab><ALLI:mode><GPI:mode>1): ... this.
>> >> >> >         (tbranch<mode>4): New.
>> >> >> >
>> >> >> > gcc/testsuite/ChangeLog:
>> >> >> >
>> >> >> >         * gcc.target/aarch64/tbz_1.c: New test.
>> >> >> >
>> >> >> > --- inline copy of patch ---
>> >> >> >
>> >> >> > diff --git a/gcc/config/aarch64/aarch64.md
>> >> >> > b/gcc/config/aarch64/aarch64.md index
>> >> >> >
>> >> >>
>> >>
>> 2bc2684b82c35a44e0a2cea6e3aaf32d939f8cdf..d7684c93fba5b717d568e1a4fd
>> >> >> 71
>> >> >> > 2bde55c7c72e 100644
>> >> >> > --- a/gcc/config/aarch64/aarch64.md
>> >> >> > +++ b/gcc/config/aarch64/aarch64.md
>> >> >> > @@ -943,12 +943,29 @@ (define_insn "*cb<optab><mode>1"
>> >> >> >                       (const_int 1)))]
>> >> >> >  )
>> >> >> >
>> >> >> > -(define_insn "*tb<optab><mode>1"
>> >> >> > +(define_expand "tbranch<mode>4"
>> >> >> >    [(set (pc) (if_then_else
>> >> >> > -             (EQL (zero_extract:DI (match_operand:GPI 0
>> "register_operand"
>> >> >> "r")
>> >> >> > -                                   (const_int 1)
>> >> >> > -                                   (match_operand 1
>> >> >> > -                                     "aarch64_simd_shift_imm_<mode>" "n"))
>> >> >> > +               (match_operator 0 "aarch64_comparison_operator"
>> >> >> > +                [(match_operand:ALLI 1 "register_operand")
>> >> >> > +                 (match_operand:ALLI 2
>> >> >> "aarch64_simd_shift_imm_<ALLI:mode>")])
>> >> >> > +               (label_ref (match_operand 3 "" ""))
>> >> >> > +               (pc)))]
>> >> >> > +  "optimize > 0"
>> >> >>
>> >> >> Why's the pattern conditional on optimize?  Seems a valid choice
>> >> >> at -O0
>> >> too.
>> >> >>
>> >> >
>> >> > Hi,
>> >> >
>> >> > I had explained the reason why in the original patch, just didn't
>> >> > repeat it in
>> >> the ping:
>> >> >
>> >> > Instead of emitting the instruction directly I've chosen to expand
>> >> > the pattern using a zero extract and generating the existing
>> >> > pattern for comparisons for two
>> >> > reasons:
>> >> >
>> >> >   1. Allows for CSE of the actual comparison.
>> >> >   2. It looks like the code in expand makes the label as unused and
>> >> > removed
>> >> it
>> >> >      if it doesn't see a separate reference to it.
>> >> >
>> >> > Because of this expansion though I disable the pattern at -O0 since
>> >> > we
>> >> have no combine in that case so we'd end up with worse code.  I did
>> >> try emitting the pattern directly, but as mentioned in no#2 expand
>> >> would then kill the label.
>> >> >
>> >> > Basically I emit the pattern directly, immediately during expand
>> >> > the label is
>> >> marked as dead for some weird reason.
>> >>
>> >> Isn't #2 a bug though?  It seems like something we should fix rather
>> >> than work around.
>> >
>> > Yes it's a bug ☹ ok if I'm going to fix that bug then do I need to
>> > split the optabs still? Isn't the problem atm that I need the split?
>> > If I'm emitting the instruction directly then the recog pattern for it
>> > can just be (eq (vec_extract x 1) 0) which is the correct semantics?
>> 
>> What rtx does the code that uses the optab pass for operand 0?
>
> It gets passed the full comparison:
>
> (eq (reg/v:SI 92 [ x ])
>     (const_int 0 [0]))
>
> of which we only look at the operator.

OK, that's what I thought.  The problem is then the one I mentioned above.
This rtx doesn't describe the operation that the optab is supposed to
perform, so it can never be used in the instruction pattern.  (This is
different from something like cbranch, where operand 0 can be used directly
if the target supports a very general compare-and-branch instruction.)

If we want to use a single optab, the code that generates the optab should
pass something like:

  (eq/ne (zero_extract op0 (const_int 1) op1) (const_int 0))

as operand 0, so that operand 0 specifies the real test condition.

Thanks,
Richard

  reply	other threads:[~2022-11-15 11:33 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-31 11:53 [PATCH 1/2]middle-end: Add new tbranch optab to add support for bit-test-and-branch operations Tamar Christina
2022-10-31 11:53 ` [PATCH 2/2]AArch64 Support new tbranch optab Tamar Christina
2022-11-14 15:58   ` Tamar Christina
2022-11-15 10:36     ` Richard Sandiford
2022-11-15 10:42       ` Tamar Christina
2022-11-15 10:50         ` Richard Sandiford
2022-11-15 11:00           ` Tamar Christina
2022-11-15 11:14             ` Richard Sandiford
2022-11-15 11:23               ` Tamar Christina
2022-11-15 11:33                 ` Richard Sandiford [this message]
2022-11-15 11:39                   ` Tamar Christina
2022-11-22 13:48                   ` Tamar Christina
2022-11-22 14:00                     ` Richard Sandiford
2022-11-24 12:18                       ` Tamar Christina
2022-12-01 16:44                         ` Tamar Christina
2022-12-05 14:06                           ` Richard Sandiford
2022-10-31 11:54 ` [PATCH]AArch64 Extend umov and sbfx patterns Tamar Christina
2022-10-31 12:26   ` Richard Sandiford
2022-11-11 14:42     ` Tamar Christina
2022-11-15 11:10       ` Richard Sandiford
2022-10-31 21:16 ` [PATCH 1/2]middle-end: Add new tbranch optab to add support for bit-test-and-branch operations Jeff Law
2022-11-01 15:53   ` Tamar Christina
2022-11-01 17:00     ` Jeff Law
2022-11-02  9:55       ` Tamar Christina
2022-11-02 11:08         ` Aldy Hernandez
2022-11-05 14:23           ` Richard Biener
2022-11-14 15:56             ` Tamar Christina
2022-11-14 16:22               ` Jeff Law
2022-11-15  7:33               ` Richard Biener
2022-12-01 16:29                 ` Tamar Christina
2022-12-02  7:09                   ` Richard Biener
2022-12-05 12:00                   ` Richard Sandiford
2022-12-05 13:14                     ` 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=mptiljgjvu9.fsf@arm.com \
    --to=richard.sandiford@arm.com \
    --cc=Marcus.Shawcroft@arm.com \
    --cc=Richard.Earnshaw@arm.com \
    --cc=Tamar.Christina@arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=nd@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).