All of lore.kernel.org
 help / color / mirror / Atom feed
From: Richard Henderson <richard.henderson@linaro.org>
To: Alistair Francis <alistair.francis@wdc.com>,
	qemu-devel@nongnu.org, qemu-riscv@nongnu.org
Cc: alistair23@gmail.com, bmeng.cn@gmail.com, palmer@dabbelt.com
Subject: Re: [PATCH v1 8/8] target/riscv: Include RV32 instructions in RV64 build
Date: Tue, 6 Apr 2021 07:57:10 -0700	[thread overview]
Message-ID: <7aea1ab9-ac9b-5245-7b3a-b6d87bef6c0d@linaro.org> (raw)
In-Reply-To: <1c19d0112fae5ec6087cfc415f0d6cc56495220b.1617393702.git.alistair.francis@wdc.com>

On 4/2/21 1:03 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis<alistair.francis@wdc.com>
> ---
>   target/riscv/insn16-32.decode | 24 ++++++++++++++++++++++++
>   target/riscv/insn16-64.decode | 31 +++++++++++++++++++++++++++++++
>   target/riscv/translate.c      | 18 +++++++++++++++++-
>   target/riscv/meson.build      |  7 +++++--
>   4 files changed, 77 insertions(+), 3 deletions(-)

Having these split out from the main decode file was a cool trick when we were 
sure that we never needed to support both.  Now, I think it would be better to 
merge the patterns back in to the main insn16.decode file.

You'd begin by adding a REQUIRE_64BIT(ctx) macro, much like the other REQUIRE 
macros in translate.c, using the translate-local is_32bit() function as 
discussed earlier.

You add this to all of the trans_* functions that are currently #ifdef 
TARGET_RISCV64, and merge the insn32-64.decode bits in first.

There may well be an issue of missing helper functions, particularly when it 
comes to RVF/RVD/RVV.  There's a suggestion that I made for Claudio for i386 
that may help here:

https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg08595.html

where you add stubs that allow the gen_helper_foo() function to be declared, 
insist that it be optimized away, and reduce to an abort with -O0.

Only after insn32.decode is complete, do insn16.decode.  That's because many of 
the insns here will need the REQUIRE_64BIT in place for correctness.

E.g.

{
   ld          011  ... ... .. ... 00 @cl_d
   flw         011  ... ... .. ... 00 @cl_w
}

Since ld already has REQUIRE_64BIT, for is_32bit it returns false, and we fall 
into the flw code.

There will be one extra helper required, when the 64-bit RVC insn has a set of 
illegal operands:

static bool trans_c64_illegal(DisasContext *ctx, arg_empty *a)
{
     REQUIRE_64BIT(ctx);
     return trans_illegal(ctx, a);
}

for use with quadrant 1:

{
   c64_illegal 001 -  00000  ----- 01 # c.addiw, RES rd=0
   addiw       001 .  .....  ..... 01 @ci
   jal         001     ........... 01 @cj    rd=1  # C.JAL

}

and quadrant 2:

{
   c64_illegal 011 -  00000  ----- 10 # c.ldsp, RES rd=0
   ld          011 .  .....  ..... 10 @c_ldsp
   flw         011 .  .....  ..... 10 @c_lwsp
}


r~


WARNING: multiple messages have this Message-ID (diff)
From: Richard Henderson <richard.henderson@linaro.org>
To: Alistair Francis <alistair.francis@wdc.com>,
	qemu-devel@nongnu.org, qemu-riscv@nongnu.org
Cc: bmeng.cn@gmail.com, palmer@dabbelt.com, alistair23@gmail.com
Subject: Re: [PATCH v1 8/8] target/riscv: Include RV32 instructions in RV64 build
Date: Tue, 6 Apr 2021 07:57:10 -0700	[thread overview]
Message-ID: <7aea1ab9-ac9b-5245-7b3a-b6d87bef6c0d@linaro.org> (raw)
In-Reply-To: <1c19d0112fae5ec6087cfc415f0d6cc56495220b.1617393702.git.alistair.francis@wdc.com>

On 4/2/21 1:03 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis<alistair.francis@wdc.com>
> ---
>   target/riscv/insn16-32.decode | 24 ++++++++++++++++++++++++
>   target/riscv/insn16-64.decode | 31 +++++++++++++++++++++++++++++++
>   target/riscv/translate.c      | 18 +++++++++++++++++-
>   target/riscv/meson.build      |  7 +++++--
>   4 files changed, 77 insertions(+), 3 deletions(-)

Having these split out from the main decode file was a cool trick when we were 
sure that we never needed to support both.  Now, I think it would be better to 
merge the patterns back in to the main insn16.decode file.

You'd begin by adding a REQUIRE_64BIT(ctx) macro, much like the other REQUIRE 
macros in translate.c, using the translate-local is_32bit() function as 
discussed earlier.

You add this to all of the trans_* functions that are currently #ifdef 
TARGET_RISCV64, and merge the insn32-64.decode bits in first.

There may well be an issue of missing helper functions, particularly when it 
comes to RVF/RVD/RVV.  There's a suggestion that I made for Claudio for i386 
that may help here:

https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg08595.html

where you add stubs that allow the gen_helper_foo() function to be declared, 
insist that it be optimized away, and reduce to an abort with -O0.

Only after insn32.decode is complete, do insn16.decode.  That's because many of 
the insns here will need the REQUIRE_64BIT in place for correctness.

E.g.

{
   ld          011  ... ... .. ... 00 @cl_d
   flw         011  ... ... .. ... 00 @cl_w
}

Since ld already has REQUIRE_64BIT, for is_32bit it returns false, and we fall 
into the flw code.

There will be one extra helper required, when the 64-bit RVC insn has a set of 
illegal operands:

static bool trans_c64_illegal(DisasContext *ctx, arg_empty *a)
{
     REQUIRE_64BIT(ctx);
     return trans_illegal(ctx, a);
}

for use with quadrant 1:

{
   c64_illegal 001 -  00000  ----- 01 # c.addiw, RES rd=0
   addiw       001 .  .....  ..... 01 @ci
   jal         001     ........... 01 @cj    rd=1  # C.JAL

}

and quadrant 2:

{
   c64_illegal 011 -  00000  ----- 10 # c.ldsp, RES rd=0
   ld          011 .  .....  ..... 10 @c_ldsp
   flw         011 .  .....  ..... 10 @c_lwsp
}


r~


  reply	other threads:[~2021-04-06 14:58 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-02 20:02 [PATCH v1 0/8] RISC-V: Steps towards running 32-bit guests on Alistair Francis
2021-04-02 20:02 ` Alistair Francis
2021-04-02 20:02 ` [PATCH v1 1/8] target/riscv: Remove the hardcoded RVXLEN macro Alistair Francis
2021-04-02 20:02   ` Alistair Francis
2021-04-05 14:48   ` Richard Henderson
2021-04-05 14:48     ` Richard Henderson
2021-04-12  9:10   ` Bin Meng
2021-04-12  9:10     ` Bin Meng
2021-04-02 20:02 ` [PATCH v1 2/8] target/riscv: Remove the hardcoded SSTATUS_SD macro Alistair Francis
2021-04-02 20:02   ` Alistair Francis
2021-04-05 14:49   ` Richard Henderson
2021-04-05 14:49     ` Richard Henderson
2021-04-12  9:10   ` Bin Meng
2021-04-12  9:10     ` Bin Meng
2021-04-02 20:02 ` [PATCH v1 3/8] target/riscv: Remove the hardcoded HGATP_MODE macro Alistair Francis
2021-04-02 20:02   ` Alistair Francis
2021-04-05 14:54   ` Richard Henderson
2021-04-05 14:54     ` Richard Henderson
2021-04-02 20:02 ` [PATCH v1 4/8] target/riscv: Remove the hardcoded MSTATUS_SD macro Alistair Francis
2021-04-02 20:02   ` Alistair Francis
2021-04-05 15:10   ` Richard Henderson
2021-04-05 15:10     ` Richard Henderson
2021-04-07 17:11     ` Alistair Francis
2021-04-07 17:11       ` Alistair Francis
2021-04-08 15:20     ` Alistair Francis
2021-04-08 15:20       ` Alistair Francis
2021-04-08 18:51       ` Richard Henderson
2021-04-08 18:51         ` Richard Henderson
2021-04-02 20:02 ` [PATCH v1 5/8] target/riscv: Remove the hardcoded SATP_MODE macro Alistair Francis
2021-04-02 20:02   ` Alistair Francis
2021-04-05 15:14   ` Richard Henderson
2021-04-05 15:14     ` Richard Henderson
2021-04-02 20:02 ` [PATCH v1 6/8] target/riscv: Remove the unused HSTATUS_WPRI macro Alistair Francis
2021-04-02 20:02   ` Alistair Francis
2021-04-05 15:15   ` Richard Henderson
2021-04-05 15:15     ` Richard Henderson
2021-04-12  9:10   ` Bin Meng
2021-04-12  9:10     ` Bin Meng
2021-04-02 20:02 ` [PATCH v1 7/8] target/riscv: Remove an unused CASE_OP_32_64 macro Alistair Francis
2021-04-02 20:02   ` Alistair Francis
2021-04-05 15:15   ` Richard Henderson
2021-04-05 15:15     ` Richard Henderson
2021-04-12  9:10   ` Bin Meng
2021-04-12  9:10     ` Bin Meng
2021-04-02 20:03 ` [PATCH v1 8/8] target/riscv: Include RV32 instructions in RV64 build Alistair Francis
2021-04-02 20:03   ` Alistair Francis
2021-04-06 14:57   ` Richard Henderson [this message]
2021-04-06 14:57     ` Richard Henderson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7aea1ab9-ac9b-5245-7b3a-b6d87bef6c0d@linaro.org \
    --to=richard.henderson@linaro.org \
    --cc=alistair.francis@wdc.com \
    --cc=alistair23@gmail.com \
    --cc=bmeng.cn@gmail.com \
    --cc=palmer@dabbelt.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-riscv@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.