Re: [PING] [PATCH RESEND] riscv: improve the cost model for loading a 64bit constant in rv32.

[Jeff Law <jeffreyalaw@gmail.com>]

On 11/17/22 00:32, Lin Sinan via Gcc-patches wrote:
> The motivation of this patch is to correct the wrong estimation of
> the number of instructions needed for loading a 64bit constant in
> rv32 in the current cost model(riscv_interger_cost). According to
> the current implementation, if a constant requires more than 3
> instructions(riscv_const_insn and riscv_legitimate_constant_p),
> then the constant will be put into constant pool when expanding
> gimple to rtl(legitimate_constant_p hook and emit_move_insn).
> So the inaccurate cost model leads to the suboptimal codegen
> in rv32 and the wrong estimation part could be corrected through
> this fix.
>
> e.g. the current codegen for loading 0x839290001 in rv32
>
>    lui     a5,%hi(.LC0)
>    lw      a0,%lo(.LC0)(a5)
>    lw      a1,%lo(.LC0+4)(a5)
> .LC0:
>    .word   958988289
>    .word   8
>
> output after this patch
>
>    li a0,958988288
>    addi a0,a0,1
>    li a1,8
>
> gcc/ChangeLog:
>
>          * config/riscv/riscv.cc (riscv_build_integer): Handle the case of loading 64bit constant in rv32.
>
> gcc/testsuite/ChangeLog:
>
>          * gcc.target/riscv/rv32-load-64bit-constant.c: New test.
>
> Signed-off-by: Lin Sinan <sinan.lin@linux.alibaba.com>
> ---
>   gcc/config/riscv/riscv.cc                     | 23 +++++++++++
>   .../riscv/rv32-load-64bit-constant.c          | 38 +++++++++++++++++++
>   2 files changed, 61 insertions(+)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/rv32-load-64bit-constant.c
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 32f9ef9ade9..9dffabdc5e3 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -618,6 +618,29 @@ riscv_build_integer (struct riscv_integer_op *codes, HOST_WIDE_INT value,
>   	}
>       }
>   
> +  if ((value > INT32_MAX || value < INT32_MIN) && !TARGET_64BIT)

Nit.   It's common practice to have the TARGET test first in a series of 
tests.  It may also be advisable to break this into two lines.  
Something like this:


   if ((!TARGET_64BIT)
       || value > INT32_MAX || value < INT32_MIN)


That's the style most GCC folks are more accustomed to reading.



> +    {
> +      unsigned HOST_WIDE_INT loval = sext_hwi (value, 32);
> +      unsigned HOST_WIDE_INT hival = sext_hwi ((value - loval) >> 32, 32);
> +      struct riscv_integer_op alt_codes[RISCV_MAX_INTEGER_OPS],
> +			    hicode[RISCV_MAX_INTEGER_OPS];
> +      int hi_cost, lo_cost;
> +
> +      hi_cost = riscv_build_integer_1 (hicode, hival, mode);
> +      if (hi_cost < cost)
> +	{
> +	  lo_cost = riscv_build_integer_1 (alt_codes, loval, mode);
> +	  if (lo_cost + hi_cost < cost)

Just so I'm sure.  "cost" here refers strictly to other synthesized 
forms? If so, then ISTM that we'd want to generate the new style when 
lo_cost + hi_cost < cost OR when lo_cost + hi_cost is less than loading 
the constant from memory -- which is almost certainly more than "3" 
since the sequence from memory will be at least 3 instructions, two of 
which will hit memory.


Jeff