[PATCH]AArch64: Fix codegen regressions around tbz.

[Richard Sandiford]

Tamar Christina <tamar.christina@arm.com> writes:
> Hi All,
>
> We were analyzing code quality after recent changes and have noticed that the
> tbz support somehow managed to increase the number of branches overall rather
> than decreased them.
>
> While investigating this we figured out that the problem is that when an
> existing & <contants> exists in gimple and the instruction is generated because
> of the range information gotten from the ANDed constant that we end up with the
> situation that you get a NOP AND in the RTL expansion.
>
> This is not a problem as CSE will take care of it normally.   The issue is when
> this original AND was done in a location where PRE or FRE "lift" the AND to a
> different basic block.  This triggers a problem when the resulting value is not
> single use.  Instead of having an AND and tbz, we end up generating an
> AND + TST + BR if the mode is HI or QI.
>
> This CSE across BB was a problem before but this change made it worse. Our
> branch patterns rely on combine being able to fold AND or zero_extends into the
> instructions.
>
> To work around this (since a proper fix is outside of the scope of stage-4) we
> are limiting the new tbranch optab to only HI and QI mode values.  This isn't a
> problem because these two modes are modes for which we don't have CBZ support,
> so they are the problematic cases to begin with.  Additionally booleans are QI.
>
> The second thing we're doing is limiting the only legal bitpos to pos 0. i.e.
> only the bottom bit.  This such that we prevent the double ANDs as much as
> possible.
>
> Now most other cases, i.e. where we had an explicit & in the source code are
> still handled correctly by the anonymous (*tb<optab><ALLI:mode><GPI:mode>1)
> pattern that was added along with tbranch support.
>
> This means we don't expand the superflous AND here, and while it doesn't fix the
> problem that in the cross BB case we loss tbz, it also doesn't make things worse.
>
> With these tweaks we've now reduced the number of insn uniformly as originally
> expected.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64.md (tbranch_<code><mode>3): Restrict to SHORT
> 	and bottom bit only.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/tbz_2.c: New test.

Agreed that reducing the scope of the new optimisation seems like a safe
compromise for GCC 13.  But could you add a testcase that shows the
effect of both changes (reducing the mode selection and the bit
selection)?  The test above passes even without the patch.

It would be good to have a PR tracking this too.

Personally, I think we should try to get to the stage where gimple
does the CSE optimisations we need, and where the tbranch optab can
generate a tbz directly (rather than splitting it apart and hoping
that combine will put it back together later).

Thanks,
Richard

> --- inline copy of patch -- 
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 2c1367977a68fc8e4289118e07bb61398856791e..aa09e93d85e9628e8944e03498697eb9597ef867 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -949,8 +949,8 @@ (define_insn "*cb<optab><mode>1"
>  
>  (define_expand "tbranch_<code><mode>3"
>    [(set (pc) (if_then_else
> -              (EQL (match_operand:ALLI 0 "register_operand")
> -                   (match_operand 1 "aarch64_simd_shift_imm_<mode>"))
> +              (EQL (match_operand:SHORT 0 "register_operand")
> +                   (match_operand 1 "const0_operand"))
>                (label_ref (match_operand 2 ""))
>                (pc)))]
>    ""
> diff --git a/gcc/testsuite/gcc.target/aarch64/tbz_2.c b/gcc/testsuite/gcc.target/aarch64/tbz_2.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ec128b58f35276a7c5452685a65c73f95f2d5f9a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/tbz_2.c
> @@ -0,0 +1,130 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O2 -std=c99  -fno-unwind-tables -fno-asynchronous-unwind-tables" } */
> +/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */
> +
> +#include <stdbool.h>
> +
> +void h(void);
> +
> +/*
> +** g1:
> +** 	cbnz	w0, .L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g1(int x)
> +{
> +  if (__builtin_expect (x, 0))
> +    h ();
> +}
> +
> +/* 
> +** g2:
> +** 	tbnz	x0, 0, .L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g2(int x)
> +{
> +  if (__builtin_expect (x & 1, 0))
> +    h ();
> +}
> +
> +/* 
> +** g3:
> +** 	tbnz	x0, 3, .L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g3(int x)
> +{
> +  if (__builtin_expect (x & 8, 0))
> +    h ();
> +}
> +
> +/* 
> +** g4:
> +** 	tbnz	w0, #31, .L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g4(int x)
> +{
> +  if (__builtin_expect (x & (1 << 31), 0))
> +    h ();
> +}
> +
> +/* 
> +** g5:
> +** 	tst	w0, 255
> +** 	bne	.L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g5(char x)
> +{
> +  if (__builtin_expect (x, 0))
> +    h ();
> +}
> +
> +/* 
> +** g6:
> +** 	tbnz	w0, 0, .L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g6(char x)
> +{
> +  if (__builtin_expect (x & 1, 0))
> +    h ();
> +}
> +
> +/* 
> +** g7:
> +** 	tst	w0, 3
> +** 	bne	.L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g7(char x)
> +{
> +  if (__builtin_expect (x & 3, 0))
> +    h ();
> +}
> +
> +/* 
> +** g8:
> +** 	tbnz	w0, 7, .L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g8(char x)
> +{
> +  if (__builtin_expect (x & (1 << 7), 0))
> +    h ();
> +}
> +
> +/* 
> +** g9:
> +** 	tbnz	w0, 0, .L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g9(bool x)
> +{
> +  if (__builtin_expect (x, 0))
> +    h ();
> +}
> +
> +/* 
> +** g10:
> +** 	tbnz	w0, 0, .L[0-9]+
> +** 	ret
> +** 	...
> +*/
> +void g10(bool x)
> +{
> +  if (__builtin_expect (x & 1, 0))
> +    h ();
> +}
> +