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

[Tamar Christina <Tamar.Christina@arm.com>]

> -----Original Message-----
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: Tuesday, November 15, 2022 11:34 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 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.)

So I was wrong before about which RTL it gets passed.  Deep in the expansion
Code the rtl operation 

(eq (reg/v:SI 92 [ x ])
      (const_int 0 [0]))

Gets broken up and passed piecewise.

First thing it does it explicitly check that the first argument in RTL is an operator:

gcc_assert (insn_operand_matches (icode, 0, test));

and then the jump is emitted by breaking apart the rtl into it's operands:

4646      insn = emit_jump_insn (GEN_FCN (icode) (test, XEXP (test, 0),
4647                          XEXP (test, 1), label));

And so the operands are:

>>> p debug (operand0)
(reg/v:SI 92 [ xD.4391 ])

>>> p debug (operand1)
(const_int 0 [0])

>>> p debug (operand2)
(code_label 0 0 0 2 (nil) [0 uses])

And targets never get to see the equality check.  If the documentation of the optab is
Updated to say that the target operand1 is to be used in a zero_extract with operand0
and compared with 0 then that should be fine no?  that's the semantic of the optab itself.

Based on that I don't think we need to split this optab do we?  Just update the docs to
clarify the zero extract semantics?

Thanks,
Tamar

> 
> 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