qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 25/30] target/ppc: Move ADDI, ADDIS to decodetree, impleme


From: Matheus K. Ferst
Subject: Re: [PATCH v3 25/30] target/ppc: Move ADDI, ADDIS to decodetree, implement PADDI
Date: Fri, 30 Apr 2021 11:05:56 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1

On 29/04/2021 22:15, Richard Henderson wrote:
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
  target/ppc/insn32.decode                   | 12 +++++++
  target/ppc/insn64.decode                   | 15 +++++++++
  target/ppc/translate.c                     | 29 ----------------
  target/ppc/translate/fixedpoint-impl.c.inc | 39 ++++++++++++++++++++++
  4 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index b175441209..52d9b355d4 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -16,3 +16,15 @@
  # You should have received a copy of the GNU Lesser General Public
  # License along with this library; if not, see <http://www.gnu.org/licenses/>.
  #
+
+&D              rt ra si
+@D              ...... rt:5 ra:5 si:s16                 &D
+
+# If a prefix is allowed, decode with default values.
+&PLS_D          rt ra si:int64_t r:bool
+@PLS_D          ...... rt:5 ra:5 si:s16                 &PLS_D r=0
+
+### Fixed-Point Arithmetic Instructions
+
+ADDI            001110 ..... ..... ................     @PLS_D
+ADDIS           001111 ..... ..... ................     @D
diff --git a/target/ppc/insn64.decode b/target/ppc/insn64.decode
index 9fc45d0614..f4272df724 100644
--- a/target/ppc/insn64.decode
+++ b/target/ppc/insn64.decode
@@ -16,3 +16,18 @@
  # You should have received a copy of the GNU Lesser General Public
  # License along with this library; if not, see <http://www.gnu.org/licenses/>.
  #
+
+# Many all of these instruction names would be prefixed by "P",
+# but we share code with the non-prefixed instruction.
+
+# Format MLS:D and 8LS:D
+&PLS_D          rt ra si:int64_t r:bool  !extern
+%pls_si         32:s18 0:16
+@PLS_D          ...... .. ... r:1 .. .................. \
+                ...... rt:5 ra:5 ................       \
+                &PLS_D si=%pls_si
+
+### Fixed-Point Arithmetic Instructions
+
+ADDI            000001 10 0--.-- ..................     \
+                001110 ..... ..... ................     @PLS_D

I'm not sure about this. It's a bit surprising to find ADDI here, and the comment that explains why is likely to be ignored after the big copyright header.

<snip>

diff --git a/target/ppc/translate/fixedpoint-impl.c.inc 
b/target/ppc/translate/fixedpoint-impl.c.inc
index b740083605..7af1b3bcf5 100644
--- a/target/ppc/translate/fixedpoint-impl.c.inc
+++ b/target/ppc/translate/fixedpoint-impl.c.inc
@@ -16,3 +16,42 @@
   * You should have received a copy of the GNU Lesser General Public
   * License along with this library; if not, see 
<http://www.gnu.org/licenses/>.
   */
+
+/*
+ * Incorporate CIA into the constant when R=1.
+ * Validate that when R=1, RA=0.
+ */
+static bool resolve_PLS_D(DisasContext *ctx, arg_PLS_D *a)
+{
+    if (a->r) {
+        if (unlikely(a->ra != 0)) {
+            gen_invalid(ctx);
+            return false;
+        }
+        a->si += ctx->cia;
+    }
+    return true;
+}
+
+static bool trans_ADDI(DisasContext *ctx, arg_PLS_D *a)
+{
+    if (resolve_PLS_D(ctx, a)) {
+        if (a->ra) {
+            tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_gpr[a->ra], a->si);
+        } else {
+            tcg_gen_movi_tl(cpu_gpr[a->rt], a->si);
+        }
+    }
+    return true;
+}
+

I'd prefer to keep a trans_PADDI like

> static bool trans_PADDI(DisasContext *ctx, arg_PLS_D *a)
> {
>     if(!resolve_PLS_D(ctx, a)) {
>         return false;
>     }
>     return trans_ADDI(ctx, a);
> }

It's the middle way between v2 and v3. trans_ADDI code is reused, it'll probably be optimized as a tail call, and resolve_PLS_D is not called when it's not needed.

+static bool trans_ADDIS(DisasContext *ctx, arg_D *a)
+{
+    int si = a->si << 16;
+    if (a->ra) {
+        tcg_gen_addi_tl(cpu_gpr[a->rt], cpu_gpr[a->ra], si);
+    } else {
+        tcg_gen_movi_tl(cpu_gpr[a->rt], si);
+    }
+    return true;
+}


I'd also keep this as in the last version, where trans_ADDI is called.

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software JĂșnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]