Re: [PATCH 2/2]AArch64 Support new tbranch optab.

[Richard Sandiford <richard.sandiford@arm.com>]

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