[prev in list] [next in list] [prev in thread] [next in thread] 

List:       binutils
Subject:    Re: [PATCH] RISC-V: Improve li expansion for better code density.
From:       Jim Wilson <jimw () sifive ! com>
Date:       2019-08-23 2:09:38
Message-ID: CAFyWVaa46Wm=6mCgAEb3M78__s+AXrsrptsu8xT3eF7m9wkp0A () mail ! gmail ! com
[Download RAW message or body]

On Wed, Aug 21, 2019 at 11:40 PM Kito Cheng <kito.cheng@sifive.com> wrote:
> -       macro_build (&lower, "addi", "d,s,j", reg, reg, BFD_RELOC_RISCV_LO12_I);
> +       md_assemblef ("%s x%d, x%d, %" BFD_VMA_FMT "d", ADD32_INSN, reg, reg,
> +                     lower.X_add_number);

You are replacing addi with ADD32_INSN which maps to an addiw on a
64-bit target.  addiw always sign-extends the 32-bit result, and hence
can't be used to load a 64-bit constant, which is what this code is
doing.  Fixing this requires fixing the 64-bit testcases.

> +         /* Dicard low part and zero-extend uppper immediate.  */

uppper -> upper

> -       macro_build (ep, ADD32_INSN, "d,s,j", reg, hi_reg,
> -                    BFD_RELOC_RISCV_LO12_I);
> +       md_assemblef ("%s x%d, x%d, %" BFD_VMA_FMT "d", ADD32_INSN, reg, hi_reg,
> +                     lower.X_add_number);

This one should be ADD32_INSN, because it is loading a 32-bit
constant, which requires addiw on a 64-bit target, so this is correct.

Otherwise, this looks good with the changes Andreas and Andrew asked
for in addition to the changes I asked for.

FYI a testcase to show this is broken.  This works with an unpatched
binutils.  WIth your patch I get this result

gamma05:2022$ cat tmp.s
.file "tmp.c"
.option nopic
.attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
.attribute unaligned_access, 0
.attribute stack_align, 16
.text
.section .rodata.str1.8,"aMS",@progbits,1
.align 3
.LC0:
.string "0x%lx\n"
.text
.align 1
.globl main
.type main, @function
main:
addi sp,sp,-16
sd ra,8(sp)
li a1,0x1234567887654321
lui a0,%hi(.LC0)
addi a0,a0,%lo(.LC0)
call printf
li a0,0
ld ra,8(sp)
addi sp,sp,16
jr ra
.size main, .-main
.ident "GCC: (GNU) 9.2.0"
gamma05:2023$ build-install/bin/riscv64-unknown-elf-gcc tmp.s
gamma05:2024$ build-install/bin/qemu-riscv64 ./a.out
0xffffffff87654321
gamma05:2025$

We can't put an execution testcase like this into the binutils
testsuite though.  I just created a small C testcase, and then edited
the .s file to add a 64-bit li macro instruction.

Jim
[prev in list] [next in list] [prev in thread] [next in thread]