[RFC] RISC-V: elide sign extend when expanding cmp_and_jump

[Vineet Gupta]

On 10/24/23 22:01, Vineet Gupta wrote:
> RV64 comapre and branch instructions only support 64-bit operands.
> The backend unconditionally generates zero/sign extend at Expand time
> for compare and branch operands even if they are already as such
> e.g. function args which ABI guarantees to be sign-extended (at callsite).
>
> And subsequently REE fails to eliminate them as
> 	"missing defintion(s)"
> specially for function args since they show up as missing explicit
> definition.
>
> So elide the sign extensions at Expand time for a subreg promoted var
> with inner word sized value whcih doesn't need explicit sign extending
> (fairly good represntative of an incoming function arg).
>
> There are patches floating to enhance REE and/or new passes to elimiate
> extensions, but it is always better to not generate them if possible,
> given Expand is so early, the elimiated extend would help improve outcomes
> of later passes such as combine if they have fewer operations/combinations
> to deal with.
>
> The test used was existing gcc.c-torture/execute/20020506-1.c -O2 zbb
> It elimiates the SEXT.W and an additional branch
>
> before					after
> -------					------
> test2:					test2:
> 	sext.b	a0,a0
> 	blt	a0,zero,.L15
> 	bne	a1,zero,.L17			bne     a1,zero,.L17
>
> This is marked RFC as I only ran a spot check with a directed test and
> want to use Patrick's pre-commit CI to do the A/B testing for me.
>
> gcc/ChangeLog:
> 	* config/riscv/riscv.cc (riscv_extend_comparands): Don't
> 	sign-extend operand if subreg promoted with inner word size.
>
> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> ---
>   gcc/config/riscv/riscv.cc | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index f2dcb0db6fbd..a8d12717e43d 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -3707,7 +3707,16 @@ riscv_extend_comparands (rtx_code code, rtx *op0, rtx *op1)
>   	}
>         else
>   	{
> -	  *op0 = gen_rtx_SIGN_EXTEND (word_mode, *op0);
> +	/* A subreg promoted SI of DI could be peeled to expose DI, eliding
> +	   an unnecessary sign extension.  */
> +	if (GET_CODE (*op0) == SUBREG
> +	    && SUBREG_PROMOTED_VAR_P (*op0)
> +	    && GET_MODE_SIZE (GET_MODE (XEXP (*op0, 0))).to_constant ()
> +	       == GET_MODE_SIZE (word_mode))
> +	     *op0 = XEXP (*op0, 0);
> +	else
> +	     *op0 = gen_rtx_SIGN_EXTEND (word_mode, *op0);
> +
>   	  if (*op1 != const0_rtx)
>   	    *op1 = gen_rtx_SIGN_EXTEND (word_mode, *op1);
>   	}

Just a quick update that testing revealed additional failures and 
unpacking which took me a while and in hindsight embarrassing. I was 
misunderstanding what ABI guarantees and what ISA actually does :-)

The ABI guarantees sign extension this for 32-bit things in 64-bit reg 
(so in rv64, int), but *not* 8-bit things in a reg. i.e. not for char 
arguments.
e.g.

uint8_t abs8(uint8_t x)
{
    if (x & 0x80) // SEXT.b a4, a0
       ...
}

main()
{
     if (abs8(128) != 127)     // LI a0, 128  => ADDI a0, x0, 128
        __builtin_abort();
}

So my patch was optimizing away the SEXT.B (despite claiming that subreg 
prom of SI....) which is wrong.
Sure ADDI in main () sign-extends, but it does that for 12-bit imm 
0x080, which comes out to 0x80, but sign-extending for char scope 0x80 
would yield 0xFFFF....0x80. Hence the issue.

I'll spin a v2 after more testing.

This is slightly disappointing as it reduces the scope of optimization, 
but correctness in this trade is non-negotiable :-)

-Vineet