Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub
On 11/12/18 11:04 AM, Aleksandar Markovic wrote: > Hello, Richard. > > I am a little taken aback by your tone. I hope we can communicate in much > friendlier maner, as we used to do. I too was put off by your tone. Beginning with "there is no plan" and continuing with "there is no point" is a curt reply. It is off-putting to someone attempting to contribute, and I felt it was unwarranted. > I am not preventing anyone from experimenting. I just want to warn Philippe > about high-level view that the code in question, although not the nicest, > works, and is planned to be maintained with minimal changes. The focus of > MIPS target is on adding new architectures and ASEs, and (I correct myself) > it could be that decodetree would kick in in such cases - but not for mature > code driving older architectures. It just doesn't make enough sense. I think this is a mistake. The legacy code is quite tangled. It *should* have an overhaul. If someone is willing to do that work, fantastic. r~
Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub
Hello, Richard.
I am a little taken aback by your tone. I hope we can communicate in much
friendlier maner, as we used to do.
>You have just this year added yet another encoding scheme for nanomips. Your
>statement "well tested over many years" is a bit of a stretch.
I wrote "mostly stable mature", not "all stable mature".
>If you do not wish to work on this, that's your prerogative. But I don't think
you should be arbitrarily shutting down experimentation on this line before it
has a chance to show results.
I am not preventing anyone from experimenting. I just want to warn Philippe
about high-level view that the code in question, although not the nicest,
works, and is planned to be maintained with minimal changes. The focus of MIPS
target is on adding new architectures and ASEs, and (I correct myself) it could
be that decodetree would kick in in such cases - but not for mature code
driving older architectures. It just doesn't make enough sense.
Thanks,
Aleksandar
From: Richard Henderson
Sent: Monday, November 12, 2018 9:58:05 AM
To: Aleksandar Markovic; Philippe Mathieu-Daudé; Bastian Koppelmann; Peer
Adelt; Richard Henderson
Cc: qemu-devel@nongnu.org; Aurelien Jarno
Subject: Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub
On 11/12/18 6:37 AM, Aleksandar Markovic wrote:
>> Subject: [RFC PATCH 08/11] target/mips: Add a decodetree stub
>
> There is no plan to use decodetree for MIPS target. MIPS decoding engine is
> mostly stable mature code that was well tested over many years, and there is
> no
> point in introducing such drastic change to the code that works.
This attitude is unnecessarily obstructionist.
The reorganization of the instruction implementation that is implied by a
transition to decodetree is exactly what you and I talked about some months ago.
The ability to use multiple decodetree parsers to vector directly to the
instruction implementations would help unify code across multiple encoding
schemes. (There 4 now: mips, mips16, micromips, nanomips; and mipsr6 is
different enough that it could be considered a 5th encoding.)
In a sample transition of 10 insns that Phillipe sent me via private email this
weekend, I noticed an existing bug in MULT{,U} that fails to enforce rd == 0 on
cpus that have only one accumulator -- any bets there are no more to be found?
You have just this year added yet another encoding scheme for nanomips. Your
statement "well tested over many years" is a bit of a stretch.
If you do not wish to work on this, that's your prerogative. But I don't think
you should be arbitrarily shutting down experimentation on this line before it
has a chance to show results.
r~
Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub
On 11/12/18 6:37 AM, Aleksandar Markovic wrote:
>> Subject: [RFC PATCH 08/11] target/mips: Add a decodetree stub
>
> There is no plan to use decodetree for MIPS target. MIPS decoding engine is
> mostly stable mature code that was well tested over many years, and there is
> no
> point in introducing such drastic change to the code that works.
This attitude is unnecessarily obstructionist.
The reorganization of the instruction implementation that is implied by a
transition to decodetree is exactly what you and I talked about some months ago.
The ability to use multiple decodetree parsers to vector directly to the
instruction implementations would help unify code across multiple encoding
schemes. (There 4 now: mips, mips16, micromips, nanomips; and mipsr6 is
different enough that it could be considered a 5th encoding.)
In a sample transition of 10 insns that Phillipe sent me via private email this
weekend, I noticed an existing bug in MULT{,U} that fails to enforce rd == 0 on
cpus that have only one accumulator -- any bets there are no more to be found?
You have just this year added yet another encoding scheme for nanomips. Your
statement "well tested over many years" is a bit of a stretch.
If you do not wish to work on this, that's your prerogative. But I don't think
you should be arbitrarily shutting down experimentation on this line before it
has a chance to show results.
r~
Re: [Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub
> Subject: [RFC PATCH 08/11] target/mips: Add a decodetree stub
There is no plan to use decodetree for MIPS target. MIPS decoding engine is
mostly stable mature code that was well tested over many years, and there is no
point in introducing such drastic change to the code that works.
Thanks,
Aleksandar
From: Philippe Mathieu-Daudé on behalf of
Philippe Mathieu-Daudé
Sent: Monday, November 12, 2018 12:36:19 AM
To: Bastian Koppelmann; Peer Adelt; Richard Henderson
Cc: Philippe Mathieu-Daudé; qemu-devel@nongnu.org; Aurelien Jarno; Aleksandar
Markovic
Subject: [RFC PATCH 08/11] target/mips: Add a decodetree stub
Signed-off-by: Philippe Mathieu-Daudé
---
target/mips/Makefile.objs | 8
target/mips/insns.decode| 2 ++
target/mips/translate.c | 7 +++
target/mips/translate.inc.c | 13 +
4 files changed, 30 insertions(+)
create mode 100644 target/mips/insns.decode
create mode 100644 target/mips/translate.inc.c
diff --git a/target/mips/Makefile.objs b/target/mips/Makefile.objs
index 651f36f517..3510835d57 100644
--- a/target/mips/Makefile.objs
+++ b/target/mips/Makefile.objs
@@ -2,3 +2,11 @@ obj-y += translate.o dsp_helper.o op_helper.o lmi_helper.o
helper.o cpu.o
obj-y += gdbstub.o msa_helper.o mips-semi.o
obj-$(CONFIG_SOFTMMU) += machine.o cp0_timer.o
obj-$(CONFIG_KVM) += kvm.o
+
+DECODETREE = $(SRC_PATH)/scripts/decodetree.py
+
+target/mips/decode.inc.c: $(SRC_PATH)/target/mips/insns.decode $(DECODETREE)
+ $(call quiet-command,\
+ $(PYTHON) $(DECODETREE) -o $@ $<, "GEN", $(TARGET_DIR)$@)
+
+target/mips/translate.o: target/mips/decode.inc.c
diff --git a/target/mips/insns.decode b/target/mips/insns.decode
new file mode 100644
index 00..7fbf21cbb9
--- /dev/null
+++ b/target/mips/insns.decode
@@ -0,0 +1,2 @@
+# MIPS32/MIPS64 Instruction Set
+#
diff --git a/target/mips/translate.c b/target/mips/translate.c
index e726f3ec00..560325c563 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -27848,6 +27848,8 @@ static void gen_msa(CPUMIPSState *env, DisasContext
*ctx)
}
+#include "translate.inc.c"
+
static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
{
int32_t offset;
@@ -27872,6 +27874,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext
*ctx)
gen_set_label(l1);
}
+/* Transition to the auto-generated decoder. */
+if (decode(ctx, ctx->opcode)) {
+return;
+}
+
op = MASK_OP_MAJOR(ctx->opcode);
rs = (ctx->opcode >> 21) & 0x1f;
rt = (ctx->opcode >> 16) & 0x1f;
diff --git a/target/mips/translate.inc.c b/target/mips/translate.inc.c
new file mode 100644
index 00..69fe78ac89
--- /dev/null
+++ b/target/mips/translate.inc.c
@@ -0,0 +1,13 @@
+/*
+ * MIPS emulation for QEMU - MIPS32 translation routines
+ *
+ * Copyright (c) 2004-2005 Jocelyn Mayer
+ * Copyright (c) 2006 Marius Groeger (FPU operations)
+ * Copyright (c) 2006 Thiemo Seufer (MIPS32R2 support)
+ * Copyright (c) 2018 Philippe Mathieu-Daudé
+ *
+ * This code is licensed under the GNU GPLv2 and later.
+ */
+
+/* Include the auto-generated decoder. */
+#include "decode.inc.c"
--
2.17.2
[Qemu-devel] [RFC PATCH 08/11] target/mips: Add a decodetree stub
Signed-off-by: Philippe Mathieu-Daudé
---
target/mips/Makefile.objs | 8
target/mips/insns.decode| 2 ++
target/mips/translate.c | 7 +++
target/mips/translate.inc.c | 13 +
4 files changed, 30 insertions(+)
create mode 100644 target/mips/insns.decode
create mode 100644 target/mips/translate.inc.c
diff --git a/target/mips/Makefile.objs b/target/mips/Makefile.objs
index 651f36f517..3510835d57 100644
--- a/target/mips/Makefile.objs
+++ b/target/mips/Makefile.objs
@@ -2,3 +2,11 @@ obj-y += translate.o dsp_helper.o op_helper.o lmi_helper.o
helper.o cpu.o
obj-y += gdbstub.o msa_helper.o mips-semi.o
obj-$(CONFIG_SOFTMMU) += machine.o cp0_timer.o
obj-$(CONFIG_KVM) += kvm.o
+
+DECODETREE = $(SRC_PATH)/scripts/decodetree.py
+
+target/mips/decode.inc.c: $(SRC_PATH)/target/mips/insns.decode $(DECODETREE)
+ $(call quiet-command,\
+ $(PYTHON) $(DECODETREE) -o $@ $<, "GEN", $(TARGET_DIR)$@)
+
+target/mips/translate.o: target/mips/decode.inc.c
diff --git a/target/mips/insns.decode b/target/mips/insns.decode
new file mode 100644
index 00..7fbf21cbb9
--- /dev/null
+++ b/target/mips/insns.decode
@@ -0,0 +1,2 @@
+# MIPS32/MIPS64 Instruction Set
+#
diff --git a/target/mips/translate.c b/target/mips/translate.c
index e726f3ec00..560325c563 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -27848,6 +27848,8 @@ static void gen_msa(CPUMIPSState *env, DisasContext
*ctx)
}
+#include "translate.inc.c"
+
static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
{
int32_t offset;
@@ -27872,6 +27874,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext
*ctx)
gen_set_label(l1);
}
+/* Transition to the auto-generated decoder. */
+if (decode(ctx, ctx->opcode)) {
+return;
+}
+
op = MASK_OP_MAJOR(ctx->opcode);
rs = (ctx->opcode >> 21) & 0x1f;
rt = (ctx->opcode >> 16) & 0x1f;
diff --git a/target/mips/translate.inc.c b/target/mips/translate.inc.c
new file mode 100644
index 00..69fe78ac89
--- /dev/null
+++ b/target/mips/translate.inc.c
@@ -0,0 +1,13 @@
+/*
+ * MIPS emulation for QEMU - MIPS32 translation routines
+ *
+ * Copyright (c) 2004-2005 Jocelyn Mayer
+ * Copyright (c) 2006 Marius Groeger (FPU operations)
+ * Copyright (c) 2006 Thiemo Seufer (MIPS32R2 support)
+ * Copyright (c) 2018 Philippe Mathieu-Daudé
+ *
+ * This code is licensed under the GNU GPLv2 and later.
+ */
+
+/* Include the auto-generated decoder. */
+#include "decode.inc.c"
--
2.17.2
