Re: [PATCH] RISC-V: improve codegen for repeating large constants [3]

[Andrew Waterman <andrew@sifive.com>]

I don't believe this is correct; the subtraction is needed to account
for the fact that the low part might be negative, resulting in a
borrow from the high part.  See the output for your test case below:

$ cat test.c
#include <stdio.h>

int main()
{
  unsigned long result, tmp;

asm (
  "li      %1,-252645376\n"
  "addi    %1,%1,240\n"
  "slli    %0,%1,32\n"
  "add     %0,%0,%1"
    : "=r" (result), "=r" (tmp));

  printf("%lx\n", result);

  return 0;
}
$ riscv64-unknown-elf-gcc -O2 test.c
$ spike pk a.out
bbl loader
f0f0f0eff0f0f0f0
$


On Fri, Jun 30, 2023 at 4:42 PM Vineet Gupta <vineetg@rivosinc.com> wrote:
>
>
>
> On 6/30/23 16:33, Vineet Gupta wrote:
> > Ran into a minor snafu in const splitting code when playing with test
> > case from an old PR/23813.
> >
> >       long long f(void) { return 0xF0F0F0F0F0F0F0F0ull; }
> >
> > This currently generates
> >
> >       li      a5,-252645376
> >       addi    a5,a5,241
> >       li      a0,-252645376
> >       slli    a5,a5,32
> >       addi    a0,a0,240
> >       add     a0,a5,a0
> >       ret
> >
> > The signed math in hival extraction introduces an additional bit,
> > causing loval == hival check to fail.
> >
> > | riscv_split_integer (val=-1085102592571150096, mode=E_DImode) at ../gcc/config/riscv/riscv.cc:702
> > | 702   unsigned HOST_WIDE_INT loval = sext_hwi (val, 32);
> > | (gdb)n
> > | 703   unsigned HOST_WIDE_INT hival = sext_hwi ((val - loval) >> 32, 32);
> > | (gdb)
>
> FWIW (and I missed adding this observation to the changelog) I pondered
> about using unsigned loval/hival with zext_hwi() but that in certain
> cases can cause additional insns
>
> e.g. constant 0x8000_0000 is codegen to LI 1 +SLLI 31 vs, LI
> 0xFFFFFFFF_80000000
>
>
> > | 704   rtx hi = gen_reg_rtx (mode), lo = gen_reg_rtx (mode);
> > | (gdb) p/x val
> > | $2 = 0xf0f0f0f0f0f0f0f0
> > | (gdb) p/x loval
> > | $3 = 0xfffffffff0f0f0f0
> > | (gdb) p/x hival
> > | $4 = 0xfffffffff0f0f0f1
> >                         ^^^
> > Fix that by eliding the subtraction in shift.
> >
> > With patch:
> >
> >       li      a5,-252645376
> >       addi    a5,a5,240
> >       slli    a0,a5,32
> >       add     a0,a0,a5
> >       ret
> >
> > gcc/ChangeLog:
> >
> >       * config/riscv/riscv.cc (riscv_split_integer): hival computation
> >         do elide subtraction of loval.
> >       * (riscv_split_integer_cost): Ditto.
> >       * (riscv_build_integer): Ditto
> >
> > Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> > ---
> > I wasn't planning to do any more work on large const stuff, but just ran into it this
> > on a random BZ entry when trying search for redundant constant stuff.
> > The test seemed too good to pass :-)
> > ---
> >   gcc/config/riscv/riscv.cc | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 5ac187c1b1b4..377d3aac794b 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -643,7 +643,7 @@ riscv_build_integer (struct riscv_integer_op *codes, HOST_WIDE_INT value,
> >         && (value > INT32_MAX || value < INT32_MIN))
> >       {
> >         unsigned HOST_WIDE_INT loval = sext_hwi (value, 32);
> > -      unsigned HOST_WIDE_INT hival = sext_hwi ((value - loval) >> 32, 32);
> > +      unsigned HOST_WIDE_INT hival = sext_hwi (value >> 32, 32);
> >         struct riscv_integer_op alt_codes[RISCV_MAX_INTEGER_OPS];
> >         struct riscv_integer_op hicode[RISCV_MAX_INTEGER_OPS];
> >         int hi_cost, lo_cost;
> > @@ -674,7 +674,7 @@ riscv_split_integer_cost (HOST_WIDE_INT val)
> >   {
> >     int cost;
> >     unsigned HOST_WIDE_INT loval = sext_hwi (val, 32);
> > -  unsigned HOST_WIDE_INT hival = sext_hwi ((val - loval) >> 32, 32);
> > +  unsigned HOST_WIDE_INT hival = sext_hwi (val >> 32, 32);
> >     struct riscv_integer_op codes[RISCV_MAX_INTEGER_OPS];
> >
> >     cost = 2 + riscv_build_integer (codes, loval, VOIDmode);
> > @@ -700,7 +700,7 @@ static rtx
> >   riscv_split_integer (HOST_WIDE_INT val, machine_mode mode)
> >   {
> >     unsigned HOST_WIDE_INT loval = sext_hwi (val, 32);
> > -  unsigned HOST_WIDE_INT hival = sext_hwi ((val - loval) >> 32, 32);
> > +  unsigned HOST_WIDE_INT hival = sext_hwi (val >> 32, 32);
> >     rtx hi = gen_reg_rtx (mode), lo = gen_reg_rtx (mode);
> >
> >     riscv_move_integer (lo, lo, loval, mode);
>