Re: [PATCH 2/2]AArch64 Support new tbranch optab.
Tamar Christina writes:
> Hi,
>
> I hadn't received any reply so I had implemented various ways to do this
> (about 8 of them in fact).
>
> The conclusion is that no, we cannot emit one big RTL for the final
> instruction immediately.
> The reason that all comparisons in the AArch64 backend expand to separate CC
> compares, and
> separate testing of the operands is for ifcvt.
>
> The separate CC compare is needed so ifcvt can produce csel, cset etc from
> the compares. Unlike
> say combine, ifcvt can not do recog on a parallel with a clobber. Should we
> emit the instruction
> directly then ifcvt will not be able to say, make a csel, because we have no
> patterns which handle
> zero_extract and compare. (unlike combine ifcvt cannot transform the extract
> into an AND).
>
> While you could provide various patterns for this (and I did try) you end up
> with broken patterns
> because you can't add the clobber to the CC register. If you do, ifcvt recog
> fails.
>
> i.e.
>
> int
> f1 (int x)
> {
> if (x & 1)
> return 1;
> return x;
> }
>
> We lose csel here.
>
> Secondly the reason the compare with an explicit CC mode is needed is so that
> ifcvt can transform
> the operation into a version that doesn't require the flags to be set. But
> it only does so if it know
> the explicit usage of the CC reg.
>
> For instance
>
> int
> foo (int a, int b)
> {
> return ((a & (1 << 25)) ? 5 : 4);
> }
>
> Doesn't require a comparison, the optimal form is:
>
> foo(int, int):
> ubfxx0, x0, 25, 1
> add w0, w0, 4
> ret
>
> and no compare is actually needed. If you represent the instruction using an
> ANDS instead of a zero_extract
> then you get close, but you end up with an ands followed by an add, which is
> a slower operation.
>
> These two reasons are the main reasons why all comparisons in AArch64 expand
> the way they do, so tbranch
> Shouldn't do anything differently here.
Thanks for the (useful) investigation. Makes sense.
> Additionally the reason for the optab was to pass range information
> to the backend during expansion.
Yeah. But I think the fundamental reason that AArch64 defines the
optab is still that it has native support for the associated operation
(which is a good thing, an honest reason). The fact that we split it
apart for if-conversion---in a different form from normal comparisons---
is an implementation detail. So it still seems like a proper optab,
rather than a crutch to convey tree info.
> In this version however I have represented the expand using an ANDS instead.
> This allows us not to regress
> on -O0 as the previous version did.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for master?
>
> Note that this patch relies on
> https://patchwork.sourceware.org/project/gcc/patch/y1+4qitmrqhbd...@arm.com/
> which has yet to be reviewed but which cleans up extensions so they can be
> used like this.
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64.md (*tb1): Rename to...
> (*tb1): ... this.
> (tbranch_4): New.
> (zero_extend2,
> zero_extend2,
> zero_extend2): Make dynamic calls with @.
> * config/aarch64/iterators.md(ZEROM, zerom): 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
> 4c181a96e555c2a58c59fc991000b2a2fa9bd244..7ee1d01e050004e42cd2d0049f0200da71d918bb
> 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -946,12 +946,33 @@ (define_insn "*cb1"
> (const_int 1)))]
> )
>
> -(define_insn "*tb1"
> +(define_expand "tbranch_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_" "n"))
> + (EQL (match_operand:ALLI 0 "register_operand")
> + (match_operand 1 "aarch64_simd_shift_imm_"))
> + (label_ref (match_operand 2 ""))
> + (pc)))]
> + ""
> +{
> + rtx bitvalue = gen_reg_rtx (mode);
> + rtx reg = gen_reg_rtx (mode);
> + if (mode == mode)
> +reg = operands[0];
> + else
> +emit_insn (gen_zero_extend2 (mode, mode, reg, operands[0]));
I think the last five lines should just be:
rtx reg = gen_lowpart (mode, operands[0]);
using paradoxical subregs for the QI and HI cases. Using subregs should
generate better code, since the temporary runs the risk of having the
same value live in two different pseudos at the same time (one pseudo
with the original mode, one pseudo with the extended mode).
OK with that change and without the changes to the zero_extend pattern names.
Thanks,
Richard
> + rtx val = GEN_INT (1UL << UINTVAL (operands[1]));
RE: [PATCH 2/2]AArch64 Support new tbranch optab.
Hi,
I hadn't received any reply so I had implemented various ways to do this (about
8 of them in fact).
The conclusion is that no, we cannot emit one big RTL for the final instruction
immediately.
The reason that all comparisons in the AArch64 backend expand to separate CC
compares, and
separate testing of the operands is for ifcvt.
The separate CC compare is needed so ifcvt can produce csel, cset etc from the
compares. Unlike
say combine, ifcvt can not do recog on a parallel with a clobber. Should we
emit the instruction
directly then ifcvt will not be able to say, make a csel, because we have no
patterns which handle
zero_extract and compare. (unlike combine ifcvt cannot transform the extract
into an AND).
While you could provide various patterns for this (and I did try) you end up
with broken patterns
because you can't add the clobber to the CC register. If you do, ifcvt recog
fails.
i.e.
int
f1 (int x)
{
if (x & 1)
return 1;
return x;
}
We lose csel here.
Secondly the reason the compare with an explicit CC mode is needed is so that
ifcvt can transform
the operation into a version that doesn't require the flags to be set. But it
only does so if it know
the explicit usage of the CC reg.
For instance
int
foo (int a, int b)
{
return ((a & (1 << 25)) ? 5 : 4);
}
Doesn't require a comparison, the optimal form is:
foo(int, int):
ubfxx0, x0, 25, 1
add w0, w0, 4
ret
and no compare is actually needed. If you represent the instruction using an
ANDS instead of a zero_extract
then you get close, but you end up with an ands followed by an add, which is a
slower operation.
These two reasons are the main reasons why all comparisons in AArch64 expand
the way they do, so tbranch
Shouldn't do anything differently here. Additionally the reason for the optab
was to pass range information
to the backend during expansion.
In this version however I have represented the expand using an ANDS instead.
This allows us not to regress
on -O0 as the previous version did.
Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
Ok for master?
Note that this patch relies on
https://patchwork.sourceware.org/project/gcc/patch/y1+4qitmrqhbd...@arm.com/
which has yet to be reviewed but which cleans up extensions so they can be used
like this.
Thanks,
Tamar
gcc/ChangeLog:
* config/aarch64/aarch64.md (*tb1): Rename to...
(*tb1): ... this.
(tbranch_4): New.
(zero_extend2,
zero_extend2,
zero_extend2): Make dynamic calls with @.
* config/aarch64/iterators.md(ZEROM, zerom): 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
4c181a96e555c2a58c59fc991000b2a2fa9bd244..7ee1d01e050004e42cd2d0049f0200da71d918bb
100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -946,12 +946,33 @@ (define_insn "*cb1"
(const_int 1)))]
)
-(define_insn "*tb1"
+(define_expand "tbranch_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_" "n"))
+ (EQL (match_operand:ALLI 0 "register_operand")
+ (match_operand 1 "aarch64_simd_shift_imm_"))
+ (label_ref (match_operand 2 ""))
+ (pc)))]
+ ""
+{
+ rtx bitvalue = gen_reg_rtx (mode);
+ rtx reg = gen_reg_rtx (mode);
+ if (mode == mode)
+reg = operands[0];
+ else
+emit_insn (gen_zero_extend2 (mode, mode, reg, operands[0]));
+ rtx val = GEN_INT (1UL << UINTVAL (operands[1]));
+ emit_insn (gen_and3 (bitvalue, reg, val));
+ operands[1] = const0_rtx;
+ operands[0] = aarch64_gen_compare_reg (, bitvalue,
+operands[1]);
+})
+
+(define_insn "*tb1"
+ [(set (pc) (if_then_else
+ (EQL (zero_extract:GPI (match_operand:ALLI 0 "register_operand"
"r")
+(const_int 1)
+(match_operand 1
+ "aarch64_simd_shift_imm_"
"n"))
(const_int 0))
(label_ref (match_operand 2 "" ""))
(pc)))
@@ -962,15 +983,15 @@ (define_insn "*tb1"
{
if (get_attr_far_branch (insn) == 1)
return aarch64_gen_far_branch (operands, 2, "Ltb",
-"\\t%0, %1, ");
+"\\t%0, %1, ");
else
{
operands[1] = GEN_INT (HOST_WIDE_INT_1U << UINTVAL (operands[1]));
- return "tst\t%0, %1\;\t%l2";
+ return "tst\t%0, %1\;\t%l2";
}
}
else
- return "\t%0, %1, %l2";
+ return "\t%0,
RE: [PATCH 2/2]AArch64 Support new tbranch optab.
Hi, I had a question and I figured I'd easier to ask before I spend more time implementing it I had noticed that one of the other reasons that cbranch and the other optabs like cmov explicitly emit the compare separately and use combine to match up the final form is for ifcvt. In particular by expanding tbranch directly to the final RTL we lose some ifcvt because there are no patterns that can handle the new zero_extract idiom. So the three solutions I can think of are: 1. Don't expand tbranch to its final form immediately, but still use zero_extract. This regresses -O0. (but do we care?) 2. Expand tbranch with vec_extract and provide new zero_extract based rtl sequences for ifcvt. I currently tried this, and while it works, I don't fully trust the RTL. In particular unlike say, combine Ifcvt doesn't allow me to add an extra clobber to say that CC Is clobbered by the pattern. Now tbranch Itself also expands a clobber, so the RTL isn't wrong even after ifcvt, but I'm worried that the pattern can Be idiom recognized and then no clobber could be present. I could modify the recog code in ifcvt to try to Ignore clobbers during matching. 3. I could expand using AND instead of zero_extract instead. We have more patterns handling AND, but I'm not Sure If this will fix the problem entirely, but in principle could expand to what ANDS generates and recog that instead. This shouldn't regress -O0 as we wouldn't put a zero_extract explicitly in RTL (and we already have a pattern for ANDS). What do you think? I personally favor 3.. Thanks, Tamar > -Original Message- > From: Richard Sandiford > Sent: Tuesday, November 22, 2022 2:00 PM > To: Tamar Christina > Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > ; nd ; Marcus Shawcroft > > Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > > Tamar Christina writes: > >> -Original Message- > >> From: Richard Sandiford > >> Sent: Tuesday, November 15, 2022 11:34 AM > >> To: Tamar Christina > >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> ; nd ; Marcus Shawcroft > >> > >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> > >> Tamar Christina writes: > >> >> -Original Message- > >> >> From: Richard Sandiford > >> >> Sent: Tuesday, November 15, 2022 11:15 AM > >> >> To: Tamar Christina > >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> >> ; nd ; Marcus > Shawcroft > >> >> > >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> >> > >> >> Tamar Christina writes: > >> >> >> -Original Message- > >> >> >> From: Richard Sandiford > >> >> >> Sent: Tuesday, November 15, 2022 10:51 AM > >> >> >> To: Tamar Christina > >> >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> >> >> ; nd ; Marcus > >> Shawcroft > >> >> >> > >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> >> >> > >> >> >> Tamar Christina writes: > >> >> >> >> -Original Message- > >> >> >> >> From: Richard Sandiford > >> >> >> >> Sent: Tuesday, November 15, 2022 10:36 AM > >> >> >> >> To: Tamar Christina > >> >> >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> >> >> >> ; nd ; Marcus > >> >> Shawcroft > >> >> >> >> > >> >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> >> >> >> > >> >> >> >> Tamar Christina writes: > >> >> >> >> > Hello, > >> >> >> >> > > >> >> >> >> > Ping and updated patch. > >> >> >> >> > > >> >> >> >> > Bootstrapped Regtested on aarch64-none-linux-gnu and no > >> issues. > >> >> >> >> > > >> >> >> >> > Ok for master? > >> >> >> >> > > >> >> >> >> > Thanks, > >> >> >> >> > Tamar > >> >> >> >> > > >> >> >> >> > gcc/ChangeLog: > >> >> >> >> > > >> >> >> >> > * config/aarch
Re: [PATCH 2/2]AArch64 Support new tbranch optab.
Tamar Christina writes: >> -Original Message- >> From: Richard Sandiford >> Sent: Tuesday, November 15, 2022 11:34 AM >> To: Tamar Christina >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> ; nd ; Marcus Shawcroft >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> Tamar Christina writes: >> >> -Original Message- >> >> From: Richard Sandiford >> >> Sent: Tuesday, November 15, 2022 11:15 AM >> >> To: Tamar Christina >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> >> ; nd ; Marcus Shawcroft >> >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> >> >> Tamar Christina writes: >> >> >> -Original Message- >> >> >> From: Richard Sandiford >> >> >> Sent: Tuesday, November 15, 2022 10:51 AM >> >> >> To: Tamar Christina >> >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> >> >> ; nd ; Marcus >> Shawcroft >> >> >> >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> >> >> >> >> Tamar Christina writes: >> >> >> >> -Original Message- >> >> >> >> From: Richard Sandiford >> >> >> >> Sent: Tuesday, November 15, 2022 10:36 AM >> >> >> >> To: Tamar Christina >> >> >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> >> >> >> ; nd ; Marcus >> >> Shawcroft >> >> >> >> >> >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> >> >> >> >> >> >> Tamar Christina 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 (*tb1): >> >> >> >> > Rename >> >> to... >> >> >> >> > (*tb1): ... this. >> >> >> >> > (tbranch4): 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 "*cb1" >> >> >> >> > (const_int 1)))] >> >> >> >> > ) >> >> >> >> > >> >> >> >> > -(define_insn "*tb1" >> >> >> >> > +(define_expand "tbranch4" >> >> >> >> >[(set (pc) (if_then_else >> >> >> >> > - (EQL (zero_extract:DI (match_operand:GPI 0 >> >> "register_operand" >> >> >> >> "r") >> >> >> >> > - (const_int 1) >> >> >> >> > - (match_operand 1 >> >> >> >> > - >> >> >> >> > &qu
RE: [PATCH 2/2]AArch64 Support new tbranch optab.
> -Original Message- > From: Richard Sandiford > Sent: Tuesday, November 15, 2022 11:34 AM > To: Tamar Christina > Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > ; nd ; Marcus Shawcroft > > Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > > Tamar Christina writes: > >> -Original Message- > >> From: Richard Sandiford > >> Sent: Tuesday, November 15, 2022 11:15 AM > >> To: Tamar Christina > >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> ; nd ; Marcus Shawcroft > >> > >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> > >> Tamar Christina writes: > >> >> -Original Message- > >> >> From: Richard Sandiford > >> >> Sent: Tuesday, November 15, 2022 10:51 AM > >> >> To: Tamar Christina > >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> >> ; nd ; Marcus > Shawcroft > >> >> > >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> >> > >> >> Tamar Christina writes: > >> >> >> -Original Message- > >> >> >> From: Richard Sandiford > >> >> >> Sent: Tuesday, November 15, 2022 10:36 AM > >> >> >> To: Tamar Christina > >> >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> >> >> ; nd ; Marcus > >> Shawcroft > >> >> >> > >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> >> >> > >> >> >> Tamar Christina 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 (*tb1): > >> >> >> > Rename > >> to... > >> >> >> > (*tb1): ... this. > >> >> >> > (tbranch4): 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 "*cb1" > >> >> >> > (const_int 1)))] > >> >> >> > ) > >> >> >> > > >> >> >> > -(define_insn "*tb1" > >> >> >> > +(define_expand "tbranch4" > >> >> >> >[(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_" "n")) > >> >> >> > + (match_operator 0 "aarch64_comparison_operator" > >> >> >> > +[(match_operand:ALLI 1 "register_operand") > >> >> >> > + (match_operand:ALLI 2 > >> >> >> "aarch64_simd_sh
RE: [PATCH 2/2]AArch64 Support new tbranch optab.
> -Original Message- > From: Richard Sandiford > Sent: Tuesday, November 15, 2022 11:34 AM > To: Tamar Christina > Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > ; nd ; Marcus Shawcroft > > Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > > Tamar Christina writes: > >> -Original Message- > >> From: Richard Sandiford > >> Sent: Tuesday, November 15, 2022 11:15 AM > >> To: Tamar Christina > >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> ; nd ; Marcus Shawcroft > >> > >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> > >> Tamar Christina writes: > >> >> -Original Message- > >> >> From: Richard Sandiford > >> >> Sent: Tuesday, November 15, 2022 10:51 AM > >> >> To: Tamar Christina > >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> >> ; nd ; Marcus > Shawcroft > >> >> > >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> >> > >> >> Tamar Christina writes: > >> >> >> -Original Message- > >> >> >> From: Richard Sandiford > >> >> >> Sent: Tuesday, November 15, 2022 10:36 AM > >> >> >> To: Tamar Christina > >> >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> >> >> ; nd ; Marcus > >> Shawcroft > >> >> >> > >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> >> >> > >> >> >> Tamar Christina 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 (*tb1): > >> >> >> > Rename > >> to... > >> >> >> > (*tb1): ... this. > >> >> >> > (tbranch4): 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 "*cb1" > >> >> >> > (const_int 1)))] > >> >> >> > ) > >> >> >> > > >> >> >> > -(define_insn "*tb1" > >> >> >> > +(define_expand "tbranch4" > >> >> >> >[(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_" "n")) > >> >> >> > + (match_operator 0 "aarch64_comparison_operator" > >> >> >> > +[(match_operand:ALLI 1 "register_operand") > >> >> >> > + (match_operand:ALLI 2 > >> >> >> "aarch64_simd_sh
Re: [PATCH 2/2]AArch64 Support new tbranch optab.
Tamar Christina writes: >> -Original Message- >> From: Richard Sandiford >> Sent: Tuesday, November 15, 2022 11:15 AM >> To: Tamar Christina >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> ; nd ; Marcus Shawcroft >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> Tamar Christina writes: >> >> -Original Message- >> >> From: Richard Sandiford >> >> Sent: Tuesday, November 15, 2022 10:51 AM >> >> To: Tamar Christina >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> >> ; nd ; Marcus Shawcroft >> >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> >> >> Tamar Christina writes: >> >> >> -Original Message- >> >> >> From: Richard Sandiford >> >> >> Sent: Tuesday, November 15, 2022 10:36 AM >> >> >> To: Tamar Christina >> >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> >> >> ; nd ; Marcus >> Shawcroft >> >> >> >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> >> >> >> >> Tamar Christina 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 (*tb1): Rename >> to... >> >> >> > (*tb1): ... this. >> >> >> > (tbranch4): 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 "*cb1" >> >> >> > (const_int 1)))] >> >> >> > ) >> >> >> > >> >> >> > -(define_insn "*tb1" >> >> >> > +(define_expand "tbranch4" >> >> >> >[(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_" "n")) >> >> >> > + (match_operator 0 "aarch64_comparison_operator" >> >> >> > +[(match_operand:ALLI 1 "register_operand") >> >> >> > + (match_operand:ALLI 2 >> >> >> "aarch64_simd_shift_imm_")]) >> >> >> > + (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
RE: [PATCH 2/2]AArch64 Support new tbranch optab.
> -Original Message- > From: Richard Sandiford > Sent: Tuesday, November 15, 2022 11:15 AM > To: Tamar Christina > Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > ; nd ; Marcus Shawcroft > > Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > > Tamar Christina writes: > >> -Original Message- > >> From: Richard Sandiford > >> Sent: Tuesday, November 15, 2022 10:51 AM > >> To: Tamar Christina > >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> ; nd ; Marcus Shawcroft > >> > >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> > >> Tamar Christina writes: > >> >> -Original Message- > >> >> From: Richard Sandiford > >> >> Sent: Tuesday, November 15, 2022 10:36 AM > >> >> To: Tamar Christina > >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> >> ; nd ; Marcus > Shawcroft > >> >> > >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> >> > >> >> Tamar Christina 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 (*tb1): Rename > to... > >> >> > (*tb1): ... this. > >> >> > (tbranch4): 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 "*cb1" > >> >> > (const_int 1)))] > >> >> > ) > >> >> > > >> >> > -(define_insn "*tb1" > >> >> > +(define_expand "tbranch4" > >> >> >[(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_" > >> >> > "n")) > >> >> > + (match_operator 0 "aarch64_comparison_operator" > >> >> > +[(match_operand:ALLI 1 "register_operand") > >> >> > + (match_operand:ALLI 2 > >> >> "aarch64_simd_shift_imm_")]) > >> >> > + (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. Tamar. > > Richard
Re: [PATCH 2/2]AArch64 Support new tbranch optab.
Tamar Christina writes: >> -Original Message- >> From: Richard Sandiford >> Sent: Tuesday, November 15, 2022 10:51 AM >> To: Tamar Christina >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> ; nd ; Marcus Shawcroft >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> Tamar Christina writes: >> >> -Original Message- >> >> From: Richard Sandiford >> >> Sent: Tuesday, November 15, 2022 10:36 AM >> >> To: Tamar Christina >> >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw >> >> ; nd ; Marcus Shawcroft >> >> >> >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. >> >> >> >> Tamar Christina 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 (*tb1): Rename to... >> >> > (*tb1): ... this. >> >> > (tbranch4): 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 "*cb1" >> >> > (const_int 1)))] >> >> > ) >> >> > >> >> > -(define_insn "*tb1" >> >> > +(define_expand "tbranch4" >> >> >[(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_" >> >> > "n")) >> >> > + (match_operator 0 "aarch64_comparison_operator" >> >> > +[(match_operand:ALLI 1 "register_operand") >> >> > + (match_operand:ALLI 2 >> >> "aarch64_simd_shift_imm_")]) >> >> > + (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? Richard
RE: [PATCH 2/2]AArch64 Support new tbranch optab.
> -Original Message- > From: Richard Sandiford > Sent: Tuesday, November 15, 2022 10:51 AM > To: Tamar Christina > Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > ; nd ; Marcus Shawcroft > > Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > > Tamar Christina writes: > >> -Original Message- > >> From: Richard Sandiford > >> Sent: Tuesday, November 15, 2022 10:36 AM > >> To: Tamar Christina > >> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw > >> ; nd ; Marcus Shawcroft > >> > >> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab. > >> > >> Tamar Christina 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 (*tb1): Rename to... > >> > (*tb1): ... this. > >> > (tbranch4): 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 "*cb1" > >> > (const_int 1)))] > >> > ) > >> > > >> > -(define_insn "*tb1" > >> > +(define_expand "tbranch4" > >> >[(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_" > >> > "n")) > >> > + (match_operator 0 "aarch64_comparison_operator" > >> > +[(match_operand:ALLI 1 "register_operand") > >> > + (match_operand:ALLI 2 > >> "aarch64_simd_shift_imm_")]) > >> > + (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? Thanks, Tamar > > Thanks, > Richard > > > > > > Tamar. > > > >> I think the split here shows the difficulty with having a single > >> optab and a comparison operator though. operand 0 can be something > like: > >> > >> (eq x 1) > >> > >
Re: [PATCH 2/2]AArch64 Support new tbranch optab.
Tamar Christina writes:
>> -Original Message-
>> From: Richard Sandiford
>> Sent: Tuesday, November 15, 2022 10:36 AM
>> To: Tamar Christina
>> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw
>> ; nd ; Marcus Shawcroft
>>
>> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab.
>>
>> Tamar Christina 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 (*tb1): Rename to...
>> > (*tb1): ... this.
>> > (tbranch4): 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 "*cb1"
>> > (const_int 1)))]
>> > )
>> >
>> > -(define_insn "*tb1"
>> > +(define_expand "tbranch4"
>> >[(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_" "n"))
>> > + (match_operator 0 "aarch64_comparison_operator"
>> > +[(match_operand:ALLI 1 "register_operand")
>> > + (match_operand:ALLI 2
>> "aarch64_simd_shift_imm_")])
>> > + (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.
Thanks,
Richard
>
> Tamar.
>
>> I think the split here shows the difficulty with having a single optab and a
>> comparison operator though. operand 0 can be something like:
>>
>> (eq x 1)
>>
>> but we're not comparing x for equality with 1. We're testing whether bit 1
>> is
>> zero. This means that operand 0 can't be taken literally and can't be used
>> directly in insn patterns.
>>
>> In an earlier review, I'd said:
>>
>> For the TB instructions (and for other similar instructions that I've
>> seen on other architectures) it would be more useful to have a single-bit
>> test, with operand 4 specifying the bit position. Arguably it might then
>> be better to have separate eq and ne optabs, to avoid the awkward
>> doubling
>> of the operands (operand 1 contains operands 2 and 3).
>>
>> I think we should do that eq/ne split (sorry for not pushing harder for it
>> before).
>>
>> Thanks,
>> Richard
>>
>>
>>
>> > +{
>> > + rtx bitvalue = gen_reg_rtx (DImode);
>> > + rtx tmp = simplify_gen_subreg (DImode, operands[1], GET_MODE
>> > +(operands[1]), 0);
>> > + emit_insn (gen_extzv (bitvalue, tmp, const1_rtx, operands[2]));
>> > + opera
RE: [PATCH 2/2]AArch64 Support new tbranch optab.
> -Original Message-
> From: Richard Sandiford
> Sent: Tuesday, November 15, 2022 10:36 AM
> To: Tamar Christina
> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw
> ; nd ; Marcus Shawcroft
>
> Subject: Re: [PATCH 2/2]AArch64 Support new tbranch optab.
>
> Tamar Christina 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 (*tb1): Rename to...
> > (*tb1): ... this.
> > (tbranch4): 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 "*cb1"
> > (const_int 1)))]
> > )
> >
> > -(define_insn "*tb1"
> > +(define_expand "tbranch4"
> >[(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_" "n"))
> > + (match_operator 0 "aarch64_comparison_operator"
> > +[(match_operand:ALLI 1 "register_operand")
> > + (match_operand:ALLI 2
> "aarch64_simd_shift_imm_")])
> > + (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.
Tamar.
> I think the split here shows the difficulty with having a single optab and a
> comparison operator though. operand 0 can be something like:
>
> (eq x 1)
>
> but we're not comparing x for equality with 1. We're testing whether bit 1 is
> zero. This means that operand 0 can't be taken literally and can't be used
> directly in insn patterns.
>
> In an earlier review, I'd said:
>
> For the TB instructions (and for other similar instructions that I've
> seen on other architectures) it would be more useful to have a single-bit
> test, with operand 4 specifying the bit position. Arguably it might then
> be better to have separate eq and ne optabs, to avoid the awkward
> doubling
> of the operands (operand 1 contains operands 2 and 3).
>
> I think we should do that eq/ne split (sorry for not pushing harder for it
> before).
>
> Thanks,
> Richard
>
>
>
> > +{
> > + rtx bitvalue = gen_reg_rtx (DImode);
> > + rtx tmp = simplify_gen_subreg (DImode, operands[1], GET_MODE
> > +(operands[1]), 0);
> > + emit_insn (gen_extzv (bitvalue, tmp, const1_rtx, operands[2]));
> > + operands[2] = const0_rtx;
> > + operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]),
> bitvalue,
> > +operands[2]);
> > +})
> > +
> > +(define_insn "*tb1"
> > + [(set (pc) (if_then_else
> > + (EQL (zero_extract:GPI (match_operand:ALLI 0
> > "register_operand"
> "r")
> > +(const_int 1)
> > +(match_operand 1
> > +
> > +"aarch64_simd_shift_imm_" "n"))
> >(co
Re: [PATCH 2/2]AArch64 Support new tbranch optab.
Tamar Christina 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 (*tb1): Rename to...
> (*tb1): ... this.
> (tbranch4): 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..d7684c93fba5b717d568e1a4fd712bde55c7c72e
> 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -943,12 +943,29 @@ (define_insn "*cb1"
> (const_int 1)))]
> )
>
> -(define_insn "*tb1"
> +(define_expand "tbranch4"
>[(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_" "n"))
> + (match_operator 0 "aarch64_comparison_operator"
> +[(match_operand:ALLI 1 "register_operand")
> + (match_operand:ALLI 2
> "aarch64_simd_shift_imm_")])
> + (label_ref (match_operand 3 "" ""))
> + (pc)))]
> + "optimize > 0"
Why's the pattern conditional on optimize? Seems a valid choice at -O0 too.
I think the split here shows the difficulty with having a single optab
and a comparison operator though. operand 0 can be something like:
(eq x 1)
but we're not comparing x for equality with 1. We're testing whether
bit 1 is zero. This means that operand 0 can't be taken literally
and can't be used directly in insn patterns.
In an earlier review, I'd said:
For the TB instructions (and for other similar instructions that I've
seen on other architectures) it would be more useful to have a single-bit
test, with operand 4 specifying the bit position. Arguably it might then
be better to have separate eq and ne optabs, to avoid the awkward doubling
of the operands (operand 1 contains operands 2 and 3).
I think we should do that eq/ne split (sorry for not pushing harder for
it before).
Thanks,
Richard
> +{
> + rtx bitvalue = gen_reg_rtx (DImode);
> + rtx tmp = simplify_gen_subreg (DImode, operands[1], GET_MODE
> (operands[1]), 0);
> + emit_insn (gen_extzv (bitvalue, tmp, const1_rtx, operands[2]));
> + operands[2] = const0_rtx;
> + operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]), bitvalue,
> +operands[2]);
> +})
> +
> +(define_insn "*tb1"
> + [(set (pc) (if_then_else
> + (EQL (zero_extract:GPI (match_operand:ALLI 0 "register_operand"
> "r")
> +(const_int 1)
> +(match_operand 1
> + "aarch64_simd_shift_imm_"
> "n"))
>(const_int 0))
> (label_ref (match_operand 2 "" ""))
> (pc)))
> @@ -959,15 +976,15 @@ (define_insn "*tb1"
>{
> if (get_attr_far_branch (insn) == 1)
> return aarch64_gen_far_branch (operands, 2, "Ltb",
> -"\\t%0, %1, ");
> +"\\t%0, %1, ");
> else
> {
> operands[1] = GEN_INT (HOST_WIDE_INT_1U << UINTVAL (operands[1]));
> - return "tst\t%0, %1\;\t%l2";
> + return "tst\t%0, %1\;\t%l2";
> }
>}
> else
> - return "\t%0, %1, %l2";
> + return "\t%0, %1, %l2";
>}
>[(set_attr "type" "branch")
> (set (attr "length")
> diff --git a/gcc/testsuite/gcc.target/aarch64/tbz_1.c
> b/gcc/testsuite/gcc.target/aarch64/tbz_1.c
> new file mode 100644
> index
> ..86f5d3e23cf7f1ea6f3596549ce1a0cff6774463
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/tbz_1.c
> @@ -0,0 +1,95 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O2 -std=c99 -fno-unwind-tables
> -fno-asynchronous-unwind-tables" } */
> +/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */
> +
> +#include
> +
> +void h(void);
> +
> +/*
> +** g1:
> +** tbnzx[0-9]+, #?0, .L([0-9]+)
> +** ret
> +** ...
> +*/
> +void g1(bool x)
> +{
> + if (__builtin_expect (x, 0))
> +h ();
> +}
> +
> +/*
> +** g2:
> +** tbz x[0-9]+, #?0, .L([0-9]+)
> +** b h
> +** ...
> +*/
> +void g2(bool x)
> +{
> + if (__builtin_expect (x, 1))
> +h ();
> +}
> +
> +/*
> +** g3_ge:
> +** tbnzw[0-9]+, #?31, .L[0-9]+
> +** b h
> +** ...
> +*/
> +void g3_ge(int x)
> +{
> + if (__builtin_expect (x >= 0, 1))
> +h ();
> +}
> +
> +/*
> +** g3_gt:
> +** cmp w[0-9]+, 0
> +** ble .L[0-9]+
> +** b h
> +** ...
>
RE: [PATCH 2/2]AArch64 Support new tbranch optab.
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 (*tb1): Rename to...
(*tb1): ... this.
(tbranch4): 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..d7684c93fba5b717d568e1a4fd712bde55c7c72e
100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -943,12 +943,29 @@ (define_insn "*cb1"
(const_int 1)))]
)
-(define_insn "*tb1"
+(define_expand "tbranch4"
[(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_" "n"))
+ (match_operator 0 "aarch64_comparison_operator"
+[(match_operand:ALLI 1 "register_operand")
+ (match_operand:ALLI 2 "aarch64_simd_shift_imm_")])
+ (label_ref (match_operand 3 "" ""))
+ (pc)))]
+ "optimize > 0"
+{
+ rtx bitvalue = gen_reg_rtx (DImode);
+ rtx tmp = simplify_gen_subreg (DImode, operands[1], GET_MODE (operands[1]),
0);
+ emit_insn (gen_extzv (bitvalue, tmp, const1_rtx, operands[2]));
+ operands[2] = const0_rtx;
+ operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]), bitvalue,
+operands[2]);
+})
+
+(define_insn "*tb1"
+ [(set (pc) (if_then_else
+ (EQL (zero_extract:GPI (match_operand:ALLI 0 "register_operand"
"r")
+(const_int 1)
+(match_operand 1
+ "aarch64_simd_shift_imm_"
"n"))
(const_int 0))
(label_ref (match_operand 2 "" ""))
(pc)))
@@ -959,15 +976,15 @@ (define_insn "*tb1"
{
if (get_attr_far_branch (insn) == 1)
return aarch64_gen_far_branch (operands, 2, "Ltb",
-"\\t%0, %1, ");
+"\\t%0, %1, ");
else
{
operands[1] = GEN_INT (HOST_WIDE_INT_1U << UINTVAL (operands[1]));
- return "tst\t%0, %1\;\t%l2";
+ return "tst\t%0, %1\;\t%l2";
}
}
else
- return "\t%0, %1, %l2";
+ return "\t%0, %1, %l2";
}
[(set_attr "type" "branch")
(set (attr "length")
diff --git a/gcc/testsuite/gcc.target/aarch64/tbz_1.c
b/gcc/testsuite/gcc.target/aarch64/tbz_1.c
new file mode 100644
index
..86f5d3e23cf7f1ea6f3596549ce1a0cff6774463
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/tbz_1.c
@@ -0,0 +1,95 @@
+/* { dg-do compile } */
+/* { dg-additional-options "-O2 -std=c99 -fno-unwind-tables
-fno-asynchronous-unwind-tables" } */
+/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */
+
+#include
+
+void h(void);
+
+/*
+** g1:
+** tbnzx[0-9]+, #?0, .L([0-9]+)
+** ret
+** ...
+*/
+void g1(bool x)
+{
+ if (__builtin_expect (x, 0))
+h ();
+}
+
+/*
+** g2:
+** tbz x[0-9]+, #?0, .L([0-9]+)
+** b h
+** ...
+*/
+void g2(bool x)
+{
+ if (__builtin_expect (x, 1))
+h ();
+}
+
+/*
+** g3_ge:
+** tbnzw[0-9]+, #?31, .L[0-9]+
+** b h
+** ...
+*/
+void g3_ge(int x)
+{
+ if (__builtin_expect (x >= 0, 1))
+h ();
+}
+
+/*
+** g3_gt:
+** cmp w[0-9]+, 0
+** ble .L[0-9]+
+** b h
+** ...
+*/
+void g3_gt(int x)
+{
+ if (__builtin_expect (x > 0, 1))
+h ();
+}
+
+/*
+** g3_lt:
+** tbz w[0-9]+, #?31, .L[0-9]+
+** b h
+** ...
+*/
+void g3_lt(int x)
+{
+ if (__builtin_expect (x < 0, 1))
+h ();
+}
+
+/*
+** g3_le:
+** cmp w[0-9]+, 0
+** bgt .L[0-9]+
+** b h
+** ...
+*/
+void g3_le(int x)
+{
+ if (__builtin_expect (x <= 0, 1))
+h ();
+}
+
+/*
+** g5:
+** mov w[0-9]+, 65279
+** tst w[0-9]+, w[0-9]+
+** beq .L[0-9]+
+** b h
+** ...
+*/
+void g5(int x)
+{
+ if (__builtin_expect (x & 0xfeff, 1))
+h ();
+}
rb16486.patch
Description: rb16486.patch
[PATCH 2/2]AArch64 Support new tbranch optab.
Hi All,
This implements the new tbranch optab for AArch64.
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.
While doing this I noticed that the version that checks the signbit doesn't work
The reason for this looks like an incorrect pattern. The [us]fbx
instructions are defined for index + size == regiter size. They architecturally
alias to different instructions and binutils handles this correctly.
In GCC however we tried to prematurely optimize this and added a separate split
pattern. But this pattern is also missing alternatives only handling DImode.
This just removes this and relaxes the constraints on the normal bfx pattern.
Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
Ok for master?
Thanks,
Tamar
gcc/ChangeLog:
* config/aarch64/aarch64.md (*tb1): Rename to...
(*tb1): ... this.
(tbranch4): New.
(*): Rename to...
(*): ... this.
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..6a4494a9a370139313cc8e57447717aafa14da2d
100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -943,12 +943,28 @@ (define_insn "*cb1"
(const_int 1)))]
)
-(define_insn "*tb1"
+(define_expand "tbranch4"
[(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_" "n"))
+ (match_operator 0 "aarch64_comparison_operator"
+[(match_operand:ALLI 1 "register_operand")
+ (match_operand:ALLI 2 "aarch64_simd_shift_imm_")])
+ (label_ref (match_operand 3 "" ""))
+ (pc)))]
+ "optimize > 0"
+{
+ rtx bitvalue = gen_reg_rtx (DImode);
+ emit_insn (gen_extzv (bitvalue, operands[1], const1_rtx, operands[2]));
+ operands[2] = const0_rtx;
+ operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]), bitvalue,
+operands[2]);
+})
+
+(define_insn "*tb1"
+ [(set (pc) (if_then_else
+ (EQL (zero_extract:GPI (match_operand:ALLI 0 "register_operand"
"r")
+(const_int 1)
+(match_operand 1
+ "aarch64_simd_shift_imm_"
"n"))
(const_int 0))
(label_ref (match_operand 2 "" ""))
(pc)))
@@ -959,15 +975,15 @@ (define_insn "*tb1"
{
if (get_attr_far_branch (insn) == 1)
return aarch64_gen_far_branch (operands, 2, "Ltb",
-"\\t%0, %1, ");
+"\\t%0, %1, ");
else
{
operands[1] = GEN_INT (HOST_WIDE_INT_1U << UINTVAL (operands[1]));
- return "tst\t%0, %1\;\t%l2";
+ return "tst\t%0, %1\;\t%l2";
}
}
else
- return "\t%0, %1, %l2";
+ return "\t%0, %1, %l2";
}
[(set_attr "type" "branch")
(set (attr "length")
@@ -5752,39 +5768,19 @@ (define_expand ""
)
-(define_insn "*"
+(define_insn "*"
[(set (match_operand:GPI 0 "register_operand" "=r")
- (ANY_EXTRACT:GPI (match_operand:GPI 1 "register_operand" "r")
+ (ANY_EXTRACT:GPI (match_operand:ALLI 1 "register_operand" "r")
(match_operand 2
- "aarch64_simd_shift_imm_offset_" "n")
+ "aarch64_simd_shift_imm_offset_" "n")
(match_operand 3
- "aarch64_simd_shift_imm_" "n")))]
+ "aarch64_simd_shift_imm_" "n")))]
"IN_RANGE (INTVAL (operands[2]) + INTVAL (operands[3]),
-1, GET_MODE_BITSIZE (mode) - 1)"
- "bfx\\t%0, %1, %3, %2"
+1, GET_MODE_BITSIZE (mode))"
+ "bfx\\t%0, %1, %3, %2"
[(set_attr "type" "bfx")]
)
-;; When the bit position and width add up to 32 we can use a W-reg LSR
-;; instruction taking advantage of the implicit zero-extension of the X-reg.
-(define_split
- [(set (match_operand:DI 0 "register_operand")
- (zero_extract:DI (match_operand:DI 1 "register_operand")
-(match_operand 2
-
