Re: [PATCH v3] RISC-V: elide unnecessary sign extend when expanding cmp_and_jump

[Vineet Gupta <vineetg@rivosinc.com>]

On 10/30/23 13:33, Jeff Law wrote:
>
>> +/* Helper function for riscv_extend_comparands to Sign-extend the OP.
>> +   However if the OP is SI subreg promoted with an inner DI, such as
>> +       (subreg/s/v:SI (reg/v:DI) 0
>> +   just peel off the SUBREG to get DI, avoiding extraneous 
>> extension.  */
>> +
>> +static void
>> +riscv_sign_extend_if_not_subreg_prom (rtx *op)
>> +{
>> +  if (GET_MODE (*op) == SImode

So I may have been partially wrong about v2 patch being wrong and 
needing this fixup ;-) [1]

It seems we don't have to limit this to SImode. I re-read the calling 
convention doc [2] and it says this

"When passed in registers or on the stack, integer scalars narrower than XLEN
  bits are widened according to the sign of their type up to 32 bits, then
  sign-extended to XLEN bits."

This essentially means signed short, and signed char will be already 
sign-extended at caller site and need not be done in callee: Palmer 
mention in internal slack that unadorned char is unsigned on RISC-V 
hence we don't see the compiler extra work for say 
gcc.dg/torture/pr75964.c. If the test is however tweaked to use signed 
chars (or short), it seems caller is doing the work (adjusting the 
constant being passed to be a sign-extended variant).

This further validates Jeff's comment about checking for 
SUBREG_PROMOTED_SIGNED_P (it was anyhow the right thing to begin with 
anyways).

At this point I feel like I'm into splitting hairs (in vain) territory, 
as fixing this might not matter much in practice ....

I'd suppose we go ahead with the v3 with changes Jeff asked for and 
maybe do a later fixup to relax SI.

>> +      && GET_CODE (*op) == SUBREG
>> +      && SUBREG_PROMOTED_VAR_P (*op)
>> +      && GET_MODE_SIZE (GET_MODE (XEXP (*op, 0))).to_constant ()
>> +     == GET_MODE_SIZE (word_mode))
>> +    *op = XEXP (*op, 0);
>> +  else
>> +    *op = gen_rtx_SIGN_EXTEND (word_mode, *op);
> So for the wrapped test GET_MODE_SIZE stuff), add parenthesis and 
> indent the "==" clause.  ie
>
>   && (GET_MODE_SIZE (GET_MODE (XEXP (*op), 0))).to_constant ()
>       == GET_MODE_SIZE (word_mode))
>
> Don't you also need to verify that the subreg was sign extended? The 
> PROMOTED_VAR_P just notes that it was promoted, not *how* it was 
> promoted.  I think you just need to add a test like this:
>
>   && SUBREG_PROMOTED_SIGNED_P (*op)

[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634327.html
[2] 
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc 
<https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc>