Re: [PATCH]AArch64 Extend umov and sbfx patterns.

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

Tamar Christina <Tamar.Christina@arm.com> writes:
> Hi,
>
>> > --- a/gcc/config/aarch64/aarch64-simd.md
>> > +++ b/gcc/config/aarch64/aarch64-simd.md
>> > @@ -4259,7 +4259,7 @@ (define_insn
>> "*aarch64_get_lane_zero_extend<GPI:mode><VDQV_L:mode>"
>> >  ;; Extracting lane zero is split into a simple move when it is
>> > between SIMD  ;; registers or a store.
>> >  (define_insn_and_split "aarch64_get_lane<mode>"
>> > -  [(set (match_operand:<VEL> 0 "aarch64_simd_nonimmediate_operand"
>> > "=?r, w, Utv")
>> > +  [(set (match_operand:<VEL> 0 "aarch64_simd_nonimmediate_operand"
>> > + "=r, w, Utv")
>> >  	(vec_select:<VEL>
>> >  	  (match_operand:VALL_F16_FULL 1 "register_operand" "w, w, w")
>> >  	  (parallel [(match_operand:SI 2 "immediate_operand" "i, i, i")])))]
>> 
>> Which testcase does this help with?  It didn't look like the new tests do any
>> vector stuff.
>> 
>
> Right, sorry about that, splitting up my patches resulted in this sneaking in from a different series.
> Moved now.
>
>> > -(define_insn "*<ANY_EXTEND:optab><GPI:mode>_ashl<SHORT:mode>"
>> > +(define_insn "*<ANY_EXTEND:optab><GPI:mode>_ashl<ALLX:mode>"
>> >    [(set (match_operand:GPI 0 "register_operand" "=r")
>> >  	(ANY_EXTEND:GPI
>> > -	 (ashift:SHORT (match_operand:SHORT 1 "register_operand" "r")
>> > +	 (ashift:ALLX (match_operand:ALLX 1 "register_operand" "r")
>> >  		       (match_operand 2 "const_int_operand" "n"))))]
>> > -  "UINTVAL (operands[2]) < GET_MODE_BITSIZE (<SHORT:MODE>mode)"
>> > +  "UINTVAL (operands[2]) < GET_MODE_BITSIZE (<ALLX:MODE>mode)"
>> 
>> It'd be better to avoid even defining si<-si or si<-di "extensions"
>> (even though nothing should try to match them), so how about adding:
>> 
>>   <GPI:sizen> > <ALLX:sizen> &&
>> 
>> or similar to the beginning of the condition?  The conditions for the invalid
>> combos will then be provably false at compile time and the patterns will be
>> compiled out.
>> 
>
> Done.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64.md
> 	(*<ANY_EXTEND:optab><GPI:mode>_ashl<SHORT:mode>): Renamed to...
> 	(*<ANY_EXTEND:optab><GPI:mode>_ashl<ALLX:mode>): ...this.
> 	(*zero_extend<GPI:mode>_lshr<SHORT:mode>): Renamed to...
> 	(*zero_extend<GPI:mode>_lshr<ALLX:mode>): ...this.
> 	(*extend<GPI:mode>_ashr<SHORT:mode>): Rename to...
> 	(*extend<GPI:mode>_ashr<ALLX:mode>): ...this.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/aarch64/bitmove_1.c: New test.
> 	* gcc.target/aarch64/bitmove_2.c: New test.
>
> --- inline copy of patch ---
>
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index d7684c93fba5b717d568e1a4fd712bde55c7c72e..d230bbb833f97813c8371aa07b587bd8b0292cee 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -5711,40 +5711,43 @@ (define_insn "*extrsi5_insn_di"
>    [(set_attr "type" "rotate_imm")]
>  )
>  
> -(define_insn "*<ANY_EXTEND:optab><GPI:mode>_ashl<SHORT:mode>"
> +(define_insn "*<ANY_EXTEND:optab><GPI:mode>_ashl<ALLX:mode>"
>    [(set (match_operand:GPI 0 "register_operand" "=r")
>  	(ANY_EXTEND:GPI
> -	 (ashift:SHORT (match_operand:SHORT 1 "register_operand" "r")
> +	 (ashift:ALLX (match_operand:ALLX 1 "register_operand" "r")
>  		       (match_operand 2 "const_int_operand" "n"))))]
> -  "UINTVAL (operands[2]) < GET_MODE_BITSIZE (<SHORT:MODE>mode)"
> +  "<GPI:sizen> > <ALLX:sizen>
> +   && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<ALLX:MODE>mode)"
>  {
> -  operands[3] = GEN_INT (<SHORT:sizen> - UINTVAL (operands[2]));
> +  operands[3] = GEN_INT (<ALLX:sizen> - UINTVAL (operands[2]));
>    return "<su>bfiz\t%<GPI:w>0, %<GPI:w>1, %2, %3";
>  }
>    [(set_attr "type" "bfx")]
>  )
>  
> -(define_insn "*zero_extend<GPI:mode>_lshr<SHORT:mode>"
> +(define_insn "*zero_extend<GPI:mode>_lshr<ALLX:mode>"
>    [(set (match_operand:GPI 0 "register_operand" "=r")
>  	(zero_extend:GPI
> -	 (lshiftrt:SHORT (match_operand:SHORT 1 "register_operand" "r")
> -			 (match_operand 2 "const_int_operand" "n"))))]
> -  "UINTVAL (operands[2]) < GET_MODE_BITSIZE (<SHORT:MODE>mode)"
> +	 (lshiftrt:ALLX (match_operand:ALLX 1 "register_operand" "r")
> +			(match_operand 2 "const_int_operand" "n"))))]
> +  "<GPI:sizen> > <ALLX:sizen>
> +   && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<ALLX:MODE>mode)"
>  {
> -  operands[3] = GEN_INT (<SHORT:sizen> - UINTVAL (operands[2]));
> +  operands[3] = GEN_INT (<ALLX:sizen> - UINTVAL (operands[2]));
>    return "ubfx\t%<GPI:w>0, %<GPI:w>1, %2, %3";
>  }
>    [(set_attr "type" "bfx")]
>  )
>  
> -(define_insn "*extend<GPI:mode>_ashr<SHORT:mode>"
> +(define_insn "*extend<GPI:mode>_ashr<ALLX:mode>"
>    [(set (match_operand:GPI 0 "register_operand" "=r")
>  	(sign_extend:GPI
> -	 (ashiftrt:SHORT (match_operand:SHORT 1 "register_operand" "r")
> -			 (match_operand 2 "const_int_operand" "n"))))]
> -  "UINTVAL (operands[2]) < GET_MODE_BITSIZE (<SHORT:MODE>mode)"
> +	 (ashiftrt:ALLX (match_operand:ALLX 1 "register_operand" "r")
> +			(match_operand 2 "const_int_operand" "n"))))]
> +  "<GPI:sizen> > <ALLX:sizen>
> +   && UINTVAL (operands[2]) < GET_MODE_BITSIZE (<ALLX:MODE>mode)"
>  {
> -  operands[3] = GEN_INT (<SHORT:sizen> - UINTVAL (operands[2]));
> +  operands[3] = GEN_INT (<ALLX:sizen> - UINTVAL (operands[2]));
>    return "sbfx\\t%<GPI:w>0, %<GPI:w>1, %2, %3";
>  }
>    [(set_attr "type" "bfx")]
> diff --git a/gcc/testsuite/gcc.target/aarch64/bitmove_1.c b/gcc/testsuite/gcc.target/aarch64/bitmove_1.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..5ea4265f55213d7e7e5193a3a3681c9350867b50
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/bitmove_1.c
> @@ -0,0 +1,76 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O3 -std=c99" } */
> +/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */
> +
> +#include <stdint.h>
> +
> +/*
> +** sfoo6:
> +** 	asr	x0, x0, 16
> +** 	ret
> +*/
> +int64_t sfoo6 (int32_t x)
> +{
> +  return x >> 16;
> +}
> +
> +/*
> +** ufoo6:
> +** 	lsr	w0, w0, 30
> +** 	ret
> +*/
> +uint64_t ufoo6 (uint32_t x)
> +{
> +  return x >> 30;
> +}
> +
> +/*
> +** ufoo6s:
> +** 	ubfx	w0, w0, 7, 9
> +** 	ret
> +*/
> +uint32_t ufoo6s (uint16_t x)
> +{
> +  return x >> 7;
> +}
> +
> +/*
> +** ufoo6h:
> +** 	ubfx	w0, w0, 4, 4
> +** 	ret
> +*/
> +uint16_t ufoo6h (uint8_t x)
> +{
> +  return x >> 4;
> +}
> +
> +/*
> +** sfoo62:
> +** 	sbfx	x0, x0, 10, 22
> +** 	ret
> +*/
> +int64_t sfoo62 (int32_t x)
> +{
> +  return x >> 10;
> +}
> +
> +/*
> +** ufoo62:
> +** 	lsr	w0, w0, 10
> +** 	ret
> +*/
> +uint64_t ufoo62 (uint32_t x)
> +{
> +  return x >> 10;
> +}
> +
> +/*
> +** sfoo63:
> +** 	sbfx	x0, x0, 10, 22
> +** 	ret
> +*/
> +int64_t sfoo63 (int32_t x)
> +{
> +  return x >> 10;
> +}

This is the same as sfoo62, not sure if that's intentional.

> +
> diff --git a/gcc/testsuite/gcc.target/aarch64/bitmove_2.c b/gcc/testsuite/gcc.target/aarch64/bitmove_2.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..329600cb3dbecf4cdfed994f6cfdf98ab77e8a01
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/bitmove_2.c
> @@ -0,0 +1,76 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-O3 -std=c99" } */
> +/* { dg-final { check-function-bodies "**" "" "" { target { le } } } } */
> +
> +#include <stdint.h>
> +
> +/*
> +** sfoo6:
> +** 	sbfiz	x0, x0, 16, 16
> +** 	ret
> +*/
> +int64_t sfoo6 (int32_t x)
> +{
> +  return x << 16;
> +}
> +
> +/*
> +** ufoo6:
> +** 	lsl	w0, w0, 30
> +** 	ret
> +*/
> +uint64_t ufoo6 (uint32_t x)
> +{
> +  return x << 30;
> +}
> +
> +/*
> +** ufoo6s:
> +** 	ubfiz	w0, w0, 7, 16
> +** 	ret
> +*/
> +uint32_t ufoo6s (uint16_t x)
> +{
> +  return x << 7;
> +}
> +
> +/*
> +** ufoo6h:
> +** 	...
> +** 	ubfiz	w0, w0, 4, 12
> +** 	ret
> +*/
> +uint16_t ufoo6h (uint8_t x)
> +{
> +  return x << 4;
> +}

This looks odd without the ... filled in.  It raises the question why
the width is 12 bits when the original type was only 8.

> +
> +/*
> +** sfoo62:
> +** 	sbfiz	x0, x0, 10, 22
> +** 	ret
> +*/
> +int64_t sfoo62 (int32_t x)
> +{
> +  return x << 10;
> +}
> +
> +/*
> +** ufoo62:
> +** 	lsl	w0, w0, 10
> +** 	ret
> +*/
> +uint64_t ufoo62 (uint32_t x)
> +{
> +  return x << 10;
> +}
> +
> +/*
> +** sfoo63:
> +** 	sbfiz	x0, x0, 10, 22
> +** 	ret
> +*/
> +int64_t sfoo63 (int32_t x)
> +{
> +  return x << 10;
> +}

Similarly a dup of sfoo62.

OK with those things fixed, thanks.

Richard