[v2,02/29] targer/riscv: Activate decodetree and implemnt LUI & AUIPC

Commit Message

Bastian Koppelmann Oct. 20, 2018, 7:14 a.m. UTC

for now only LUI & AUIPC are decoded and translated. If decodetree fails, we
fall back to the old decoder.

Reviewed-by: Richard Henderson <[e-mail]>
Signed-off-by: Bastian Koppelmann <[e-mail]>
Signed-off-by: Peer Adelt <[e-mail]>
---
v1 -> v2:
        - ex_shift_amount returns uint32_t

 target/riscv/Makefile.objs              | 10 +++++++
 target/riscv/insn32.decode              | 30 +++++++++++++++++++++
 target/riscv/insn_trans/trans_rvi.inc.c | 35 +++++++++++++++++++++++++
 target/riscv/translate.c                | 24 ++++++++++++-----
 4 files changed, 92 insertions(+), 7 deletions(-)
 create mode 100644 target/riscv/insn32.decode
 create mode 100644 target/riscv/insn_trans/trans_rvi.inc.c

Comments

Richard Henderson Oct. 20, 2018, 7:32 p.m. UTC | #1

On 10/20/18 12:14 AM, Bastian Koppelmann wrote:
> +    static int32_t ex_shift_##amount(int imm) \

Not that it'll matter in practice, but s/int32_t/int/.

That's the type that's passed in, and it's the type that
is stored in the argument set structures.


r~

Palmer Dabbelt Oct. 25, 2018, 4:58 p.m. UTC | #2

On Sat, 20 Oct 2018 00:14:24 PDT (-0700), [e-mail] wrote:
> for now only LUI & AUIPC are decoded and translated. If decodetree fails, we
> fall back to the old decoder.
>
> Reviewed-by: Richard Henderson <[e-mail]>
> Signed-off-by: Bastian Koppelmann <[e-mail]>
> Signed-off-by: Peer Adelt <[e-mail]>
> ---
> v1 -> v2:
>         - ex_shift_amount returns uint32_t

You mean int32_t, right?  That's what the code does, and I believe that's 
what it's supposed to do according to the ISA.

>
>  target/riscv/Makefile.objs              | 10 +++++++
>  target/riscv/insn32.decode              | 30 +++++++++++++++++++++
>  target/riscv/insn_trans/trans_rvi.inc.c | 35 +++++++++++++++++++++++++
>  target/riscv/translate.c                | 24 ++++++++++++-----
>  4 files changed, 92 insertions(+), 7 deletions(-)
>  create mode 100644 target/riscv/insn32.decode
>  create mode 100644 target/riscv/insn_trans/trans_rvi.inc.c
>
> diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs
> index abd0a7cde3..ea02f9b9ef 100644
> --- a/target/riscv/Makefile.objs
> +++ b/target/riscv/Makefile.objs
> @@ -1 +1,11 @@
>  obj-y += translate.o op_helper.o helper.o cpu.o fpu_helper.o gdbstub.o pmp.o
> +
> +DECODETREE = $(SRC_PATH)/scripts/decodetree.py
> +
> +target/riscv/decode_insn32.inc.c: \
> +  $(SRC_PATH)/target/riscv/insn32.decode $(DECODETREE)
> +	$(call quiet-command, \
> +	  $(PYTHON) $(DECODETREE) -o $@ --decode decode_insn32 $<, \
> +	  "GEN", $(TARGET_DIR)$@)
> +
> +target/riscv/translate.o: target/riscv/decode_insn32.inc.c
> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> new file mode 100644
> index 0000000000..44d4e922b6
> --- /dev/null
> +++ b/target/riscv/insn32.decode
> @@ -0,0 +1,30 @@
> +#
> +# RISC-V translation routines for the RVXI Base Integer Instruction Set.
> +#
> +# Copyright (c) 2018 Peer Adelt, [e-mail]
> +#                    Bastian Koppelmann, [e-mail]
> +#
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms and conditions of the GNU General Public License,
> +# version 2 or later, as published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
> +
> +# Fields:
> +%rd        7:5
> +
> +# immediates:
> +%imm_u    12:s20                 !function=ex_shift_12
> +
> +# Formats 32:
> +@u       ....................      ..... .......         imm=%imm_u          %rd
> +
> +# *** RV32I Base Instruction Set ***
> +lui      ....................       ..... 0110111 @u
> +auipc    ....................       ..... 0010111 @u
> diff --git a/target/riscv/insn_trans/trans_rvi.inc.c b/target/riscv/insn_trans/trans_rvi.inc.c
> new file mode 100644
> index 0000000000..aee0d1637d
> --- /dev/null
> +++ b/target/riscv/insn_trans/trans_rvi.inc.c
> @@ -0,0 +1,35 @@
> +/*
> + * RISC-V translation routines for the RVXI Base Integer Instruction Set.
> + *
> + * Copyright (c) 2016-2017 Sagar Karandikar, [e-mail]
> + * Copyright (c) 2018 Peer Adelt, [e-mail]
> + *                    Bastian Koppelmann, [e-mail]
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
> + */
> +
> +static bool trans_lui(DisasContext *ctx, arg_lui *a, uint32_t insn)
> +{
> +    if (a->rd != 0) {
> +        tcg_gen_movi_tl(cpu_gpr[a->rd], a->imm);
> +    }
> +    return true;
> +}
> +
> +static bool trans_auipc(DisasContext *ctx, arg_auipc *a, uint32_t insn)
> +{
> +    if (a->rd != 0) {
> +        tcg_gen_movi_tl(cpu_gpr[a->rd], a->imm + ctx->base.pc_next);
> +    }
> +    return true;
> +}
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index e81b9f097e..65a323a201 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1667,6 +1667,19 @@ static void decode_RV32_64C(CPURISCVState *env, DisasContext *ctx)
>      }
>  }
>
> +#define EX_SH(amount) \
> +    static int32_t ex_shift_##amount(int imm) \
> +    {                                         \
> +        return imm << amount;                 \
> +    }
> +EX_SH(12)
> +
> +bool decode_insn32(DisasContext *ctx, uint32_t insn);
> +/* Include the auto-generated decoder for 32 bit insn */
> +#include "decode_insn32.inc.c"
> +/* Include insn module translation function */
> +#include "insn_trans/trans_rvi.inc.c"
> +
>  static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx)
>  {
>      int rs1;
> @@ -1687,12 +1700,6 @@ static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx)
>      imm = GET_IMM(ctx->opcode);
>
>      switch (op) {
> -    case OPC_RISC_LUI:
> -        if (rd == 0) {
> -            break; /* NOP */
> -        }
> -        tcg_gen_movi_tl(cpu_gpr[rd], sextract64(ctx->opcode, 12, 20) << 12);
> -        break;
>      case OPC_RISC_AUIPC:
>          if (rd == 0) {
>              break; /* NOP */
> @@ -1802,7 +1809,10 @@ static void decode_opc(DisasContext *ctx)
>          }
>      } else {
>          ctx->pc_succ_insn = ctx->base.pc_next + 4;
> -        decode_RV32_64G(ctx->env, ctx);
> +        if (!decode_insn32(ctx, ctx->opcode)) {
> +            /* fallback to old decoder */
> +            decode_RV32_64G(ctx->env, ctx);
> +        }
>      }
>  }

Reviewed-by: Palmer Dabbelt <[e-mail]>

How do you want to go about merging these?  It looks like it should be possible 
to merge the patch set piecemeal, which I'd actually be happy doing as it'll be 
easier to get these out for testing that way.  That way we can avoid having one 
day where everyone changes over to a new decoder.  If that's OK then I'll start 
slurping up this up into my weekly pull requests as I can review them, with the 
idea being that I won't re-order anything (in other words, I'll start at the 
first patch and take patches until I find one that isn't ready to go).

Thanks!

Bastian Koppelmann Oct. 26, 2018, 10:49 a.m. UTC | #3

On 10/25/18 6:58 PM, Palmer Dabbelt wrote:
>
> Reviewed-by: Palmer Dabbelt <[e-mail]>
>
> How do you want to go about merging these?  It looks like it should be 
> possible to merge the patch set piecemeal, which I'd actually be happy 
> doing as it'll be easier to get these out for testing that way.  That 
> way we can avoid having one day where everyone changes over to a new 
> decoder.  If that's OK then I'll start slurping up this up into my 
> weekly pull requests as I can review them, with the idea being that I 
> won't re-order anything (in other words, I'll start at the first patch 
> and take patches until I find one that isn't ready to go).


I think you can pick up everything up to the RVC conversion which still 
needs the work suggested by Richard. Thanks, for picking it up :)

Cheers,

Bastian

Richard Henderson Oct. 26, 2018, 1:58 p.m. UTC | #4

On 10/26/18 11:49 AM, Bastian Koppelmann wrote:
> I think you can pick up everything up to the RVC conversion which still needs
> the work suggested by Richard. Thanks, for picking it up :)

Even then I thought we were talking about splitting the RV64 insns
into a separate file, reducing the ifdefs, and renaming the arg-sets
to match the instruction formats described in the riscv spec.


r~

Bastian Koppelmann Oct. 26, 2018, 2:53 p.m. UTC | #5

On 10/26/18 3:58 PM, Richard Henderson wrote:
> On 10/26/18 11:49 AM, Bastian Koppelmann wrote:
>> I think you can pick up everything up to the RVC conversion which still needs
>> the work suggested by Richard. Thanks, for picking it up :)
> Even then I thought we were talking about splitting the RV64 insns
> into a separate file, reducing the ifdefs, and renaming the arg-sets
> to match the instruction formats described in the riscv spec.


Yes, you are right I forgot that.

Cheers,

Bastian

Palmer Dabbelt Oct. 26, 2018, 3:42 p.m. UTC | #6

On Fri, 26 Oct 2018 07:53:17 PDT (-0700), Bastian Koppelmann wrote:
>
> On 10/26/18 3:58 PM, Richard Henderson wrote:
>> On 10/26/18 11:49 AM, Bastian Koppelmann wrote:
>>> I think you can pick up everything up to the RVC conversion which still needs
>>> the work suggested by Richard. Thanks, for picking it up :)
>> Even then I thought we were talking about splitting the RV64 insns
>> into a separate file, reducing the ifdefs, and renaming the arg-sets
>> to match the instruction formats described in the riscv spec.
>
>
> Yes, you are right I forgot that.

OK, so I think I'll hold off for a v3, then.

Patch

diff --git a/target/riscv/Makefile.objs b/target/riscv/Makefile.objs
index abd0a7cde3..ea02f9b9ef 100644
--- a/target/riscv/Makefile.objs
+++ b/target/riscv/Makefile.objs
@@ -1 +1,11 @@ 
 obj-y += translate.o op_helper.o helper.o cpu.o fpu_helper.o gdbstub.o pmp.o
+
+DECODETREE = $(SRC_PATH)/scripts/decodetree.py
+
+target/riscv/decode_insn32.inc.c: \
+  $(SRC_PATH)/target/riscv/insn32.decode $(DECODETREE)
+	$(call quiet-command, \
+	  $(PYTHON) $(DECODETREE) -o $@ --decode decode_insn32 $<, \
+	  "GEN", $(TARGET_DIR)$@)
+
+target/riscv/translate.o: target/riscv/decode_insn32.inc.c
diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
new file mode 100644
index 0000000000..44d4e922b6
--- /dev/null
+++ b/target/riscv/insn32.decode
@@ -0,0 +1,30 @@ 
+#
+# RISC-V translation routines for the RVXI Base Integer Instruction Set.
+#
+# Copyright (c) 2018 Peer Adelt, [e-mail]
+#                    Bastian Koppelmann, [e-mail]
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms and conditions of the GNU General Public License,
+# version 2 or later, as published by the Free Software Foundation.
+#
+# This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
+
+# Fields:
+%rd        7:5
+
+# immediates:
+%imm_u    12:s20                 !function=ex_shift_12
+
+# Formats 32:
+@u       ....................      ..... .......         imm=%imm_u          %rd
+
+# *** RV32I Base Instruction Set ***
+lui      ....................       ..... 0110111 @u
+auipc    ....................       ..... 0010111 @u
diff --git a/target/riscv/insn_trans/trans_rvi.inc.c b/target/riscv/insn_trans/trans_rvi.inc.c
new file mode 100644
index 0000000000..aee0d1637d
--- /dev/null
+++ b/target/riscv/insn_trans/trans_rvi.inc.c
@@ -0,0 +1,35 @@ 
+/*
+ * RISC-V translation routines for the RVXI Base Integer Instruction Set.
+ *
+ * Copyright (c) 2016-2017 Sagar Karandikar, [e-mail]
+ * Copyright (c) 2018 Peer Adelt, [e-mail]
+ *                    Bastian Koppelmann, [e-mail]
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
+ */
+
+static bool trans_lui(DisasContext *ctx, arg_lui *a, uint32_t insn)
+{
+    if (a->rd != 0) {
+        tcg_gen_movi_tl(cpu_gpr[a->rd], a->imm);
+    }
+    return true;
+}
+
+static bool trans_auipc(DisasContext *ctx, arg_auipc *a, uint32_t insn)
+{
+    if (a->rd != 0) {
+        tcg_gen_movi_tl(cpu_gpr[a->rd], a->imm + ctx->base.pc_next);
+    }
+    return true;
+}
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index e81b9f097e..65a323a201 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1667,6 +1667,19 @@  static void decode_RV32_64C(CPURISCVState *env, DisasContext *ctx)
     }
 }
 
+#define EX_SH(amount) \
+    static int32_t ex_shift_##amount(int imm) \
+    {                                         \
+        return imm << amount;                 \
+    }
+EX_SH(12)
+
+bool decode_insn32(DisasContext *ctx, uint32_t insn);
+/* Include the auto-generated decoder for 32 bit insn */
+#include "decode_insn32.inc.c"
+/* Include insn module translation function */
+#include "insn_trans/trans_rvi.inc.c"
+
 static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx)
 {
     int rs1;
@@ -1687,12 +1700,6 @@  static void decode_RV32_64G(CPURISCVState *env, DisasContext *ctx)
     imm = GET_IMM(ctx->opcode);
 
     switch (op) {
-    case OPC_RISC_LUI:
-        if (rd == 0) {
-            break; /* NOP */
-        }
-        tcg_gen_movi_tl(cpu_gpr[rd], sextract64(ctx->opcode, 12, 20) << 12);
-        break;
     case OPC_RISC_AUIPC:
         if (rd == 0) {
             break; /* NOP */
@@ -1802,7 +1809,10 @@  static void decode_opc(DisasContext *ctx)
         }
     } else {
         ctx->pc_succ_insn = ctx->base.pc_next + 4;
-        decode_RV32_64G(ctx->env, ctx);
+        if (!decode_insn32(ctx, ctx->opcode)) {
+            /* fallback to old decoder */
+            decode_RV32_64G(ctx->env, ctx);
+        }
     }
 }