Re: [PATCH v2] RISC-V: Only relax to C.LUI when imm != 0 and rd != 0/2

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 24 October 2017 at 03:22, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> From: Andrew Waterman <andrew@sifive.com>
>
> This matches the ISA specification.  This also adds two tests: one to
> make sure the assembler rejects invalid 'c.lui's, and one to make sure
> we only relax valid 'c.lui's.
>
> bfd/ChangeLog
>
> 2017-10-23  Andrew Waterman  <andrew@sifive.com>
>
>         * elfnn-riscv.c (_bfd_riscv_relax_lui): Don't relax to c.lui
>         when rd is x0.
>
> include/ChangeLog
>
> 2017-10-23  Andrew Waterman  <andrew@sifive.com>
>
>         * opcode/riscv.h (VALID_RVC_LUI_IMM): c.lui can't load the
>         immediate 0.
>
> gas/ChangeLog
>
> 2017-10-23  Andrew Waterman  <andrew@sifive.com>
>
>         * testsuite/gas/riscv/c-lui-fail.d: New testcase.
>         gas/testsuite/gas/riscv/c-lui-fail.l: Likewise.
>         gas/testsuite/gas/riscv/c-lui-fail.s: Likewise.
>         gas/testsuite/gas/riscv/riscv.exp: Likewise.
>
> ld/ChangeLog
>
> 2017-10-23  Andrew Waterman  <andrew@sifive.com>
>
>         * ld/testsuite/ld-riscv-elf/c-lui.d: New testcase.
>         ld/testsuite/ld-riscv-elf/c-lui.s: Likewise.
>         ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp: New test suite.
> ---
>  bfd/elfnn-riscv.c                          |  5 +++--
>  gas/testsuite/gas/riscv/c-lui-fail.d       |  3 +++
>  gas/testsuite/gas/riscv/c-lui-fail.l       |  2 ++
>  gas/testsuite/gas/riscv/c-lui-fail.s       |  2 ++
>  gas/testsuite/gas/riscv/riscv.exp          |  1 +
>  include/opcode/riscv.h                     |  2 +-
>  ld/testsuite/ld-riscv-elf/c-lui.d          | 17 +++++++++++++++++
>  ld/testsuite/ld-riscv-elf/c-lui.s          |  5 +++++
>  ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 24 ++++++++++++++++++++++++
>  9 files changed, 58 insertions(+), 3 deletions(-)
>  create mode 100644 gas/testsuite/gas/riscv/c-lui-fail.d
>  create mode 100644 gas/testsuite/gas/riscv/c-lui-fail.l
>  create mode 100644 gas/testsuite/gas/riscv/c-lui-fail.s
>  create mode 100644 ld/testsuite/ld-riscv-elf/c-lui.d
>  create mode 100644 ld/testsuite/ld-riscv-elf/c-lui.s
>  create mode 100644 ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
>
> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
> index f7cdb4e2b0f9..b6e389270f59 100644
> --- a/bfd/elfnn-riscv.c
> +++ b/bfd/elfnn-riscv.c
> @@ -2988,9 +2988,10 @@ _bfd_riscv_relax_lui (bfd *abfd,
>        && VALID_RVC_LUI_IMM (RISCV_CONST_HIGH_PART (symval))
>        && VALID_RVC_LUI_IMM (RISCV_CONST_HIGH_PART (symval + ELF_MAXPAGESIZE)))
>      {
> -      /* Replace LUI with C.LUI if legal (i.e., rd != x2/sp).  */
> +      /* Replace LUI with C.LUI if legal (i.e., rd != x0 and rd != x2/sp).  */
>        bfd_vma lui = bfd_get_32 (abfd, contents + rel->r_offset);
> -      if (((lui >> OP_SH_RD) & OP_MASK_RD) == X_SP)
> +      unsigned rd = ((unsigned)lui >> OP_SH_RD) & OP_MASK_RD;
> +      if (rd == 0 || rd == X_SP)
>         return TRUE;
>
>        lui = (lui & (OP_MASK_RD << OP_SH_RD)) | MATCH_C_LUI;
> diff --git a/gas/testsuite/gas/riscv/c-lui-fail.d b/gas/testsuite/gas/riscv/c-lui-fail.d
> new file mode 100644
> index 000000000000..03e4596d2147
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/c-lui-fail.d
> @@ -0,0 +1,3 @@
> +#as: -march=rv32ic
> +#source: c-lui-fail.s
> +#error-output: c-lui-fail.l
> diff --git a/gas/testsuite/gas/riscv/c-lui-fail.l b/gas/testsuite/gas/riscv/c-lui-fail.l
> new file mode 100644
> index 000000000000..5a4e9907dd69
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/c-lui-fail.l
> @@ -0,0 +1,2 @@
> +.*: Assembler messages:
> +.*: Error: illegal operands `c.lui x1,0'
> diff --git a/gas/testsuite/gas/riscv/c-lui-fail.s b/gas/testsuite/gas/riscv/c-lui-fail.s
> new file mode 100644
> index 000000000000..bb669bb2b7ec
> --- /dev/null
> +++ b/gas/testsuite/gas/riscv/c-lui-fail.s
> @@ -0,0 +1,2 @@
> +target:
> +       c.lui x1, 0
> diff --git a/gas/testsuite/gas/riscv/riscv.exp b/gas/testsuite/gas/riscv/riscv.exp
> index 005238f9a386..f411335bfbec 100644
> --- a/gas/testsuite/gas/riscv/riscv.exp
> +++ b/gas/testsuite/gas/riscv/riscv.exp
> @@ -21,4 +21,5 @@
>  if [istarget riscv*-*-*] {
>      run_dump_test "t_insns"
>      run_dump_test "fmv.x"
> +    run_dump_test "c-lui-fail"
>  }
> diff --git a/include/opcode/riscv.h b/include/opcode/riscv.h
> index 719565d1ba53..015e78131495 100644
> --- a/include/opcode/riscv.h
> +++ b/include/opcode/riscv.h
> @@ -141,7 +141,7 @@ static const char * const riscv_pred_succ[16] =
>  #define VALID_UTYPE_IMM(x) (EXTRACT_UTYPE_IMM(ENCODE_UTYPE_IMM(x)) == (x))
>  #define VALID_UJTYPE_IMM(x) (EXTRACT_UJTYPE_IMM(ENCODE_UJTYPE_IMM(x)) == (x))
>  #define VALID_RVC_IMM(x) (EXTRACT_RVC_IMM(ENCODE_RVC_IMM(x)) == (x))
> -#define VALID_RVC_LUI_IMM(x) (EXTRACT_RVC_LUI_IMM(ENCODE_RVC_LUI_IMM(x)) == (x))
> +#define VALID_RVC_LUI_IMM(x) (ENCODE_RVC_LUI_IMM(x) != 0 && EXTRACT_RVC_LUI_IMM(ENCODE_RVC_LUI_IMM(x)) == (x))
>  #define VALID_RVC_SIMM3(x) (EXTRACT_RVC_SIMM3(ENCODE_RVC_SIMM3(x)) == (x))
>  #define VALID_RVC_ADDI4SPN_IMM(x) (EXTRACT_RVC_ADDI4SPN_IMM(ENCODE_RVC_ADDI4SPN_IMM(x)) == (x))
>  #define VALID_RVC_ADDI16SP_IMM(x) (EXTRACT_RVC_ADDI16SP_IMM(ENCODE_RVC_ADDI16SP_IMM(x)) == (x))
> diff --git a/ld/testsuite/ld-riscv-elf/c-lui.d b/ld/testsuite/ld-riscv-elf/c-lui.d
> new file mode 100644
> index 000000000000..7a9671163a9d
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/c-lui.d
> @@ -0,0 +1,17 @@
> +#name: lui to c.lui relaxation
> +#source: c-lui.s
> +#as: -march=rv32ic
> +#ld: -shared -melf32lriscv
> +#objdump: -d -M no-aliases,numeric
> +
> +.*:     file format .*
> +
> +
> +Disassembly of section \.text:
> +
> +.* <.text>:
> +.*:    6085                    c.lui   x1,0x1
> +.*:    000000b7                lui     x1,0x0
> +.*:    00001037                lui     x0,0x1
> +.*:    00001137                lui     x2,0x1
> +#pass
> diff --git a/ld/testsuite/ld-riscv-elf/c-lui.s b/ld/testsuite/ld-riscv-elf/c-lui.s
> new file mode 100644
> index 000000000000..4a23fdb95c56
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/c-lui.s
> @@ -0,0 +1,5 @@
> +.text
> +       lui x1, 1
> +       lui x1, 0
> +       lui x0, 1
> +       lui x2, 1
> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> new file mode 100644
> index 000000000000..efe012efec7d
> --- /dev/null
> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp
> @@ -0,0 +1,24 @@
> +# Expect script for RISC-V ELF linker tests
> +#   Copyright (C) 2017 Free Software Foundation, Inc.
> +#
> +# This file is part of the GNU Binutils.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
> +# MA 02110-1301, USA.
> +#
> +
> +if [is_target "riscv-*-*"] {
> +    run_dump_test "c-lui"
> +}

Hi,

Since you committed this patch, I've noticed:
ERROR:    ld:(DejaGnu) proc "is_target riscv-*-*" does not exist.

on all the targets I test (x86_64, aarch64, arm)

Can you fix it?

Thanks,

Christophe

> --
> 2.13.6
>