[Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Philippe Mathieu-Daudé]

The three-operand MADD and MADDU are specific to the
Toshiba TX19/TX39/TX79 cores.

The "32-Bit TX System RISC TX39 Family Architecture manual"
is available at https://wiki.qemu.org/File:DSAE0022432.pdf

Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
---
Based-on: 540683755a2357b670b107a29658531466be18f4.1539428198.git.noring@nocrew.org
"target/mips: Limited support for the R5900" from Fredrik Noring:
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg02731.html

 target/mips/translate.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 2d5c0a8173..4b3168961c 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -3843,6 +3843,46 @@ static void gen_mul_txx9(DisasContext *ctx, uint32_t opc,
             tcg_temp_free_i32(t3);
         }
         break;
+    case OPC_MADD:
+        {
+            TCGv_i64 t2 = tcg_temp_new_i64();
+            TCGv_i64 t3 = tcg_temp_new_i64();
+
+            tcg_gen_ext_tl_i64(t2, t0);
+            tcg_gen_ext_tl_i64(t3, t1);
+            tcg_gen_mul_i64(t2, t2, t3);
+            tcg_gen_concat_tl_i64(t3, cpu_LO[acc], cpu_HI[acc]);
+            tcg_gen_add_i64(t2, t2, t3);
+            tcg_temp_free_i64(t3);
+            gen_move_low32(cpu_LO[acc], t2);
+            gen_move_high32(cpu_HI[acc], t2);
+            if (rd) {
+                gen_move_low32(cpu_gpr[rd], t2);
+            }
+            tcg_temp_free_i64(t2);
+        }
+        break;
+    case OPC_MADDU:
+        {
+            TCGv_i64 t2 = tcg_temp_new_i64();
+            TCGv_i64 t3 = tcg_temp_new_i64();
+
+            tcg_gen_ext32u_tl(t0, t0);
+            tcg_gen_ext32u_tl(t1, t1);
+            tcg_gen_extu_tl_i64(t2, t0);
+            tcg_gen_extu_tl_i64(t3, t1);
+            tcg_gen_mul_i64(t2, t2, t3);
+            tcg_gen_concat_tl_i64(t3, cpu_LO[acc], cpu_HI[acc]);
+            tcg_gen_add_i64(t2, t2, t3);
+            tcg_temp_free_i64(t3);
+            gen_move_low32(cpu_LO[acc], t2);
+            gen_move_high32(cpu_HI[acc], t2);
+            if (rd) {
+                gen_move_low32(cpu_gpr[rd], t2);
+            }
+            tcg_temp_free_i64(t2);
+        }
+        break;
     default:
         MIPS_INVAL("mul TXx9");
         generate_exception_end(ctx, EXCP_RI);
-- 
2.17.1

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Fredrik Noring]

Hi Philippe,

> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -3843,6 +3843,46 @@ static void gen_mul_txx9(DisasContext *ctx, uint32_t opc,

What about documenting MADD and MADDU along with MULT and MULTU in the
note above?

> +    case OPC_MADD:

This case is unreachable, because gen_mul_txx9 will never be called for
OPC_MADD.

> +            TCGv_i64 t2 = tcg_temp_new_i64();
> +            TCGv_i64 t3 = tcg_temp_new_i64();

The MADD (and MADDU) instructions are defined to multiply 32-bit integers
in the C790 manual. Are 64-bit integers required to perform this with QEMU?

> +            gen_move_low32(cpu_LO[acc], t2);
> +            gen_move_high32(cpu_HI[acc], t2);
> +            if (rd) {
> +                gen_move_low32(cpu_gpr[rd], t2);

Are LO, HI and GPR[rd] sign-extended to 64 bits when required?

> +    case OPC_MADDU:

As above, this case is unreachable, because gen_mul_txx9 will never be
called for OPC_MADDU.

> +            gen_move_low32(cpu_LO[acc], t2);
> +            gen_move_high32(cpu_HI[acc], t2);
> +            if (rd) {
> +                gen_move_low32(cpu_gpr[rd], t2);

As above, are LO, HI and GPR[rd] sign-extended to 64 bits when required?

Fredrik

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Philippe Mathieu-Daudé]

Hi Fredrik,

On Sun, Oct 14, 2018 at 6:41 PM Fredrik Noring <noring@nocrew.org> wrote:
>
> Hi Philippe,
>
> > --- a/target/mips/translate.c
> > +++ b/target/mips/translate.c
> > @@ -3843,6 +3843,46 @@ static void gen_mul_txx9(DisasContext *ctx, uint32_t opc,
>
> What about documenting MADD and MADDU along with MULT and MULTU in the
> note above?

OK.

>
> > +    case OPC_MADD:
>
> This case is unreachable, because gen_mul_txx9 will never be called for
> OPC_MADD.

Well... I was going to send my R3900 branch but you sent your R5900
branch which clashes with mine for a single #define.
I posted a series to avoid this clash [*]: "mips: Increase the
insn_flags holder size and clean mips-defs.h" so we could both work
together.
Unfortunately Aleksandar prefer I don't base my series on top of
yours, so I'm blocked waiting your to enter.

I did rebase mine on yours to avoid duplicated efforts, and your
series was easier to review, less than 10 patchs.
Since I'm modelling a SoC (QEMU system part) mine is quite bigger.

I then realized the 3.0 merge window is closing (so I now have to wait
for the 3.1 to open in January), and I might have less time to spend
on this.
So I started to look at what patches I could salvage (a patch open
posted on the mailing list is still better than a phantom one IMHO,
even if WIP or invalid).

That's true it is not reachable, it lacks the INSN_R3900 definition,
used by the R3900 mips_def_t.

I'll stop bothering with this until the code is reachable (my branch posted).

[*] https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg04072.html

>
> > +            TCGv_i64 t2 = tcg_temp_new_i64();
> > +            TCGv_i64 t3 = tcg_temp_new_i64();
>
> The MADD (and MADDU) instructions are defined to multiply 32-bit integers
> in the C790 manual. Are 64-bit integers required to perform this with QEMU?

tcg_gen_mul_i32() store the multiplication result in a 32bit register.

If you look at "Table 3-8. Multiply, multiply / add instructions
(R3000A extended instruction set)":

  MADD rd, rs, rt
  MADD rs, rt
  Multiply the contents of registers rs and rt as two’s complement
integers, and add the doubleword
  (64-bit) result to multiply/divide registers HI and LO. Also, store
the lower 32 bits of the add result
  in register rd. In the MADD rs, rt format, the store operation to a
general register is omitted.

To be able to use the 64-bit result we need to use tcg_gen_mul_i64().

> > +            gen_move_low32(cpu_LO[acc], t2);
> > +            gen_move_high32(cpu_HI[acc], t2);
> > +            if (rd) {
> > +                gen_move_low32(cpu_gpr[rd], t2);
>
> Are LO, HI and GPR[rd] sign-extended to 64 bits when required?

tcg_gen_mul_i64() seems to use unsigned internally (calling
tcg_gen_mulu2_i32()).

So this one might be wrong.
(Richard can you confirm?)

>
> > +    case OPC_MADDU:
>
> As above, this case is unreachable, because gen_mul_txx9 will never be
> called for OPC_MADDU.
>
> > +            gen_move_low32(cpu_LO[acc], t2);
> > +            gen_move_high32(cpu_HI[acc], t2);
> > +            if (rd) {
> > +                gen_move_low32(cpu_gpr[rd], t2);
>
> As above, are LO, HI and GPR[rd] sign-extended to 64 bits when required?

  MADDU rd, rs, rt
  MADDU rs, rt
  Multiply the contents of registers rs and rt as unsigned integers,
and add the doubleword (64-bit)
  result to multiply/divide registers HI and LO. Also, store the lower
32 bits of the add result in register
  rd. In the MADDU rs, rt format, the store operation to a general
register is omitted.

This one looks correct.

>
> Fredrik

Thanks for your review,

Phil.

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Maciej W. Rozycki]

On Sun, 14 Oct 2018, Philippe Mathieu-Daudé wrote:

> > > +            gen_move_low32(cpu_LO[acc], t2);
> > > +            gen_move_high32(cpu_HI[acc], t2);
> > > +            if (rd) {
> > > +                gen_move_low32(cpu_gpr[rd], t2);
> >
> > As above, are LO, HI and GPR[rd] sign-extended to 64 bits when required?
> 
>   MADDU rd, rs, rt
>   MADDU rs, rt
>   Multiply the contents of registers rs and rt as unsigned integers,
> and add the doubleword (64-bit)
>   result to multiply/divide registers HI and LO. Also, store the lower
> 32 bits of the add result in register
>   rd. In the MADDU rs, rt format, the store operation to a general
> register is omitted.
> 
> This one looks correct.

 Obviously with processors such as the TX49 or the TX79 where GPRs are 
64-bit for the purpose of integer arithmetic (i.e. any multimedia 
extension aside) any 32-bit results written to an integer register are 
implicitly sign-extended to the full 64-bit width of the destination 
register, as per the requirements of the MIPS architecture.  Otherwise 
operation of any subsequent 32-bit instruction (other than SLL or SLLV) 
using that that result as an input operand would be unpredictable.  You 
need to respect that in QEMU too.

 So results of individual operations are as in the comments with this 
code:

	mthi	$0		# HI <- 0
	mtlo	$0		# LO <- 0
	addiu	$2, $0, 1	# $2 <- 1
	lui	$3, 0x4000	# $3 <- 0x40000000
	maddu	$4, $3, $2	# HI <- 0
				# LO <- 0x40000000
				# $4 <- 0x40000000
	maddu	$5, $4, $2	# HI <- 0
				# LO <- 0xffffffff80000000
				# $5 <- 0xffffffff80000000
	maddu	$6, $4, $2	# HI <- 1
				# LO <- 0
				# $6 <- 0

Similarly:

	addiu	$2, $0, 1	# $2 <- 1
	sll	$3, $2, 31	# $3 <- 0xffffffff80000000
	dsll	$4, $2, 31	# $4 <- 0x80000000
	dsll	$5, $3, 0	# $5 <- 0xffffffff80000000
	sll	$6, $3, 0	# $6 <- 0xffffffff80000000
	dsll	$7, $4, 0	# $7 <- 0x80000000
	sll	$8, $4, 0	# $8 <- 0xffffffff80000000
	daddu	$9, $3, $3	# $9 <- 0xffffffff00000000
	addu	$10, $3, $3	# $10 <- 0
	daddu	$11, $4, $4	# $11 <- 0x100000000 
	addu	$12, $4, $4	# unpredictable!

 HTH,

  Maciej

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Fredrik Noring]

Hi Philippe,

> That's true it is not reachable, it lacks the INSN_R3900 definition,
> used by the R3900 mips_def_t.
> 
> I'll stop bothering with this until the code is reachable (my branch posted).

I would be happy if your patch could be merged soon. Adding the following
five lines to it would make both MADD and MADDU immediately reachable and
testable, at least for the R5900 to begin with:

--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -22768,6 +22768,11 @@ static void decode_opc_special2_legacy(CPUMIPSState *env, DisasContext *ctx)
     switch (op1) {
     case OPC_MADD: /* Multiply and add/sub */
     case OPC_MADDU:
+        if (ctx->insn_flags & INSN_R5900) {
+            gen_mul_txx9(ctx, op1, 0, rd, rs, rt);
+            break;
+        }
+        /* Fallthrough */
     case OPC_MSUB:
     case OPC_MSUBU:
         check_insn(ctx, ISA_MIPS32);

> To be able to use the 64-bit result we need to use tcg_gen_mul_i64().

tcg_gen_muls2_i32 produces a 64-bit result in two 32-bit registers, and
tcg_gen_add2_i32 seems to correspond to adding such registers. And then
tcg_gen_ext_i32_tl could be used to extend the results to 64 bits. Look
at the cases for MULT and MULTU, since they are very similar.

Fredrik

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Fredrik Noring]

Hi Maciej, Philippe,

>  So results of individual operations are as in the comments with this 
> code:
> 
> 	mthi	$0		# HI <- 0
> 	mtlo	$0		# LO <- 0
> 	addiu	$2, $0, 1	# $2 <- 1
> 	lui	$3, 0x4000	# $3 <- 0x40000000
> 	maddu	$4, $3, $2	# HI <- 0
> 				# LO <- 0x40000000
> 				# $4 <- 0x40000000
> 	maddu	$5, $4, $2	# HI <- 0
> 				# LO <- 0xffffffff80000000
> 				# $5 <- 0xffffffff80000000
> 	maddu	$6, $4, $2	# HI <- 1
> 				# LO <- 0
> 				# $6 <- 0

Adding tests such as the one above to for example tests/tcg/mips/txx9/
would be useful, I think. As previously noted, GCC can produce 32-bit
R5900 programs but 64-bit compilations fail, so full test coverage may
not be easily obtainable at the moment.

Fredrik

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Sent: Monday, October 15, 2018 7:02 PM
> To: Maciej W. Rozycki
> Cc: Philippe Mathieu-Daudé; Richard Henderson; Aleksandar Markovic; Aurelien Jarno; > qemu-devel@nongnu.org Developers; Jürgen Urban
> Subject: Re: [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU
> 
> Hi Maciej, Philippe,
> 
> >  So results of individual operations are as in the comments with this
> > code:
> >
> >       mthi    $0              # HI <- 0
> >       mtlo    $0              # LO <- 0
> >       addiu   $2, $0, 1       # $2 <- 1
> >       lui     $3, 0x4000      # $3 <- 0x40000000
> >       maddu   $4, $3, $2      # HI <- 0
> >                               # LO <- 0x40000000
> >                               # $4 <- 0x40000000
> >       maddu   $5, $4, $2      # HI <- 0
> >                               # LO <- 0xffffffff80000000
> >                               # $5 <- 0xffffffff80000000
> >       maddu   $6, $4, $2      # HI <- 1
> >                               # LO <- 0
> >                               # $6 <- 0
> 
> Adding tests such as the one above to for example tests/tcg/mips/txx9/
> would be useful, I think.

By no means, yes.

A peculiar case of DMULTU would be interesting.

It would be nice to implement just a single instruction from MMI, let's say PAND, and have a test for it.

Thanks,
Aleksandar

> As previously noted, GCC can produce 32-bit
> R5900 programs but 64-bit compilations fail, so full test coverage may
> not be easily obtainable at the moment.
> 
> Fredrik
> 

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Fredrik Noring]

Hi Aleksandar,

> A peculiar case of DMULTU would be interesting.

Agreed, DMULTU would be good to test as well. (DMULTU isn't part of the
R5900 ISA, though.)

> It would be nice to implement just a single instruction from MMI, let's
> say PAND, and have a test for it.

Most if not all multimedia instructions operate on 128-bit GPRs, which
means all GPRs need to be extended. I suppose there are several ways to
implement this. The definitions in target/mips/translate.c are:

/* global register indices */
static TCGv cpu_gpr[32], cpu_PC;
static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC];

One option is to create a new array such as

static TCGv_i64 mmi_gpr[32];

that represents the upper 64 bits of each GPR. Then cpu_gpr must be of
a 64-bit type too, even when QEMU runs in 32-bit user mode. The R5900
does not implement CP0.Status.UX in hardware, though, so system mode is
64 bits, regardless.

Interestingly, LO and HI are also extended to 128 bits, where the upper
64 bits are used for the I1 pipeline instructions MULT1, etc.

Additionally, a special SA register contains the shift amount used by
the 256-bit funnel shift multimedia instruction QFSRV.

What are your thoughts on making these register extensions in QEMU?

Fredrik

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Richard Henderson]

On 10/16/18 11:19 AM, Fredrik Noring wrote:
> /* global register indices */
> static TCGv cpu_gpr[32], cpu_PC;
> static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC];
> 
> One option is to create a new array such as
> 
> static TCGv_i64 mmi_gpr[32];
> 
> that represents the upper 64 bits of each GPR. Then cpu_gpr must be of
> a 64-bit type too, even when QEMU runs in 32-bit user mode. The R5900
> does not implement CP0.Status.UX in hardware, though, so system mode is
> 64 bits, regardless.

I would not implement r5900 for mips32 in that case,
I would implement it only for TARGET_MIPS64.


r~

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Fredrik Noring]

Hi Richard,

> > /* global register indices */
> > static TCGv cpu_gpr[32], cpu_PC;
> > static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC];
> > 
> > One option is to create a new array such as
> > 
> > static TCGv_i64 mmi_gpr[32];
> > 
> > that represents the upper 64 bits of each GPR. Then cpu_gpr must be of
> > a 64-bit type too, even when QEMU runs in 32-bit user mode. The R5900
> > does not implement CP0.Status.UX in hardware, though, so system mode is
> > 64 bits, regardless.
> 
> I would not implement r5900 for mips32 in that case,
> I would implement it only for TARGET_MIPS64.

R5900 Linux implements the O32 ABI, which is why 32-bit QEMU user-mode is
very useful. Perhaps a better alternative is to define the MMI registers
as 128-bit, similar to

static TCGv_u128 mmi_gpr[32];

and then copy cpu_gpr to/from mmi_gpr as needed when running the MMIs?

Fredrik

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Maciej W. Rozycki]

On Tue, 16 Oct 2018, Fredrik Noring wrote:

> One option is to create a new array such as
> 
> static TCGv_i64 mmi_gpr[32];
> 
> that represents the upper 64 bits of each GPR. Then cpu_gpr must be of
> a 64-bit type too, even when QEMU runs in 32-bit user mode. The R5900
> does not implement CP0.Status.UX in hardware, though, so system mode is
> 64 bits, regardless.

 It's more like modern CP0.Status.PX however, as the hardware does not 
implement 64-bit memory segments and only has legacy 32-bit segments 
implemented.

 Due to a hardware quirk however the value recorded in the target register 
(usually $ra) does not get sign-extended with linked jump or branch 
instructions, contrary to what is expected with processors implementing 
32-bit segments only.  This has implications for kernel code running from 
KSEG0/KSEG1/KSEG2 and has to be worked around in software, as experience 
has shown.

  Maciej

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Maciej W. Rozycki]

On Tue, 16 Oct 2018, Fredrik Noring wrote:

> > I would not implement r5900 for mips32 in that case,
> > I would implement it only for TARGET_MIPS64.
> 
> R5900 Linux implements the O32 ABI, which is why 32-bit QEMU user-mode is
> very useful. Perhaps a better alternative is to define the MMI registers
> as 128-bit, similar to
> 
> static TCGv_u128 mmi_gpr[32];
> 
> and then copy cpu_gpr to/from mmi_gpr as needed when running the MMIs?

 FWIW, I agree as far as the user emulation mode is concerned.  All 64-bit 
MIPS hardware is currently set up by the Linux kernel for 64-bit execution 
by keeping CP0.Status.UX set when running o32 user processes anyway.

 A change to this policy (and also the use of CP0.Status.PX for n32) has 
been discussed, in partictular in the course of investigating address 
space overflows caused by GCC using the indexed addressing modes under the 
assumption that the address space wraps at 32 bits for o32 and n32 
software, where indeed it does not.  No change has been implemented 
though.

  Maciej

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Aleksandar Markovic]

> Perhaps a better alternative is to define the MMI registers as 128-bit, similar to
>
> static TCGv_u128 mmi_gpr[32];
>
> and then copy cpu_gpr to/from mmi_gpr as needed when running the MMIs?

Fredrik, hi.

I think this is fine. In any case, this could be changed, if we hit any obstacle related to this format later on during development.

But you don't have to spend much time on this right now. The definition of MMI registers is more 'nice-to-have' than a 'must'.

Regards,
Aleksandar

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Fredrik Noring]

Hi Philippe,

> The three-operand MADD and MADDU are specific to the
> Toshiba TX19/TX39/TX79 cores.
> 
> The "32-Bit TX System RISC TX39 Family Architecture manual"
> is available at https://wiki.qemu.org/File:DSAE0022432.pdf
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>

I'm queueing your MADD and MADDU patch, with minor modifications as shown
below, to amend the support for the R5900, based on Aleksandar's latest
v2 tag:

https://github.com/AMarkovic/qemu tags/mips-queue-oct-2018-part-2-v2

It looks like gen_move_{low32,high32} do sign-extend properly. However,
their notes say "sign-extract" as in:

/* Sign-extract the low 32-bits to a target_long.  */
static inline void gen_move_low32(TCGv ret, TCGv_i64 arg)
...
/* Sign-extract the high 32-bits to a target_long.  */
static inline void gen_move_high32(TCGv ret, TCGv_i64 arg)

Perhaps these are typos? Also, looking at the code for tcg_gen_mulu2_i32
and tcg_gen_add2_i32, they don't appear to be particularly more efficient
anyway, in particular since more registers are needed, so let's go with
your version. (A subsequent patch will do MADD1 and MADDU1 as well.)

Thanks!

Fredrik

--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4801,8 +4801,8 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
 }
 
 /*
- * These MULT and MULTU instructions implemented in for example the
- * Toshiba/Sony R5900 and the Toshiba TX19, TX39 and TX79 core
+ * These MULT[U] and MADD[U] instructions implemented in for example
+ * the Toshiba/Sony R5900 and the Toshiba TX19, TX39 and TX79 core
  * architectures are special three-operand variants with the syntax
  *
  *     MULT[U][1] rd, rs, rt
@@ -4811,6 +4811,14 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
  *
  *     (rd, LO, HI) <- rs * rt
  *
+ * and
+ *
+ *     MADD[U]    rd, rs, rt
+ *
+ * such that
+ *
+ *     (rd, LO, HI) <- (LO, HI) + rs * rt
+ *
  * where the low-order 32-bits of the result is placed into both the
  * GPR rd and the special register LO. The high-order 32-bits of the
  * result is placed into the special register HI.
@@ -4867,8 +4875,48 @@ static void gen_mul_txx9(DisasContext *ctx, uint32_t opc,
             tcg_temp_free_i32(t3);
         }
         break;
+    case TX79_MMI_MADD:
+        {
+            TCGv_i64 t2 = tcg_temp_new_i64();
+            TCGv_i64 t3 = tcg_temp_new_i64();
+
+            tcg_gen_ext_tl_i64(t2, t0);
+            tcg_gen_ext_tl_i64(t3, t1);
+            tcg_gen_mul_i64(t2, t2, t3);
+            tcg_gen_concat_tl_i64(t3, cpu_LO[acc], cpu_HI[acc]);
+            tcg_gen_add_i64(t2, t2, t3);
+            tcg_temp_free_i64(t3);
+            gen_move_low32(cpu_LO[acc], t2);
+            gen_move_high32(cpu_HI[acc], t2);
+            if (rd) {
+                gen_move_low32(cpu_gpr[rd], t2);
+            }
+            tcg_temp_free_i64(t2);
+        }
+        break;
+    case TX79_MMI_MADDU:
+        {
+            TCGv_i64 t2 = tcg_temp_new_i64();
+            TCGv_i64 t3 = tcg_temp_new_i64();
+
+            tcg_gen_ext32u_tl(t0, t0);
+            tcg_gen_ext32u_tl(t1, t1);
+            tcg_gen_extu_tl_i64(t2, t0);
+            tcg_gen_extu_tl_i64(t3, t1);
+            tcg_gen_mul_i64(t2, t2, t3);
+            tcg_gen_concat_tl_i64(t3, cpu_LO[acc], cpu_HI[acc]);
+            tcg_gen_add_i64(t2, t2, t3);
+            tcg_temp_free_i64(t3);
+            gen_move_low32(cpu_LO[acc], t2);
+            gen_move_high32(cpu_HI[acc], t2);
+            if (rd) {
+                gen_move_low32(cpu_gpr[rd], t2);
+            }
+            tcg_temp_free_i64(t2);
+        }
+        break;
     default:
-        MIPS_INVAL("mul TXx9");
+        MIPS_INVAL("mul/madd TXx9");
         generate_exception_end(ctx, EXCP_RI);
         goto out;
     }
@@ -24699,6 +24747,8 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx)
         break;
     case TX79_MMI_MULT1:
     case TX79_MMI_MULTU1:
+    case TX79_MMI_MADD:
+    case TX79_MMI_MADDU:
         gen_mul_txx9(ctx, opc, rd, rs, rt);
         break;
     case TX79_MMI_DIV1:
@@ -24713,8 +24763,6 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx)
     case TX79_MMI_MFHI1:
         gen_HILO(ctx, opc, 1, rd);
         break;
-    case TX79_MMI_MADD:          /* TODO: TX79_MMI_MADD */
-    case TX79_MMI_MADDU:         /* TODO: TX79_MMI_MADDU */
     case TX79_MMI_PLZCW:         /* TODO: TX79_MMI_PLZCW */
     case TX79_MMI_MADD1:         /* TODO: TX79_MMI_MADD1 */
     case TX79_MMI_MADDU1:        /* TODO: TX79_MMI_MADDU1 */

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Aleksandar Markovic]

> I'm queueing your MADD and MADDU patch...

You don't queue, you submit.

Thanks,
Aleksandar

________________________________________
From: Fredrik Noring <noring@nocrew.org>
Sent: Wednesday, October 24, 2018 8:01:15 PM
To: Philippe Mathieu-Daudé
Cc: Maciej W . Rozycki; Richard Henderson; Aleksandar Markovic; Aurelien Jarno; qemu-devel@nongnu.org; Jürgen Urban
Subject: Re: [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

Hi Philippe,

> The three-operand MADD and MADDU are specific to the
> Toshiba TX19/TX39/TX79 cores.
>
> The "32-Bit TX System RISC TX39 Family Architecture manual"
> is available at https://wiki.qemu.org/File:DSAE0022432.pdf
>
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>

I'm queueing your MADD and MADDU patch, with minor modifications as shown
below, to amend the support for the R5900, based on Aleksandar's latest
v2 tag:

https://github.com/AMarkovic/qemu tags/mips-queue-oct-2018-part-2-v2

It looks like gen_move_{low32,high32} do sign-extend properly. However,
their notes say "sign-extract" as in:

/* Sign-extract the low 32-bits to a target_long.  */
static inline void gen_move_low32(TCGv ret, TCGv_i64 arg)
...
/* Sign-extract the high 32-bits to a target_long.  */
static inline void gen_move_high32(TCGv ret, TCGv_i64 arg)

Perhaps these are typos? Also, looking at the code for tcg_gen_mulu2_i32
and tcg_gen_add2_i32, they don't appear to be particularly more efficient
anyway, in particular since more registers are needed, so let's go with
your version. (A subsequent patch will do MADD1 and MADDU1 as well.)

Thanks!

Fredrik

--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4801,8 +4801,8 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
 }

 /*
- * These MULT and MULTU instructions implemented in for example the
- * Toshiba/Sony R5900 and the Toshiba TX19, TX39 and TX79 core
+ * These MULT[U] and MADD[U] instructions implemented in for example
+ * the Toshiba/Sony R5900 and the Toshiba TX19, TX39 and TX79 core
  * architectures are special three-operand variants with the syntax
  *
  *     MULT[U][1] rd, rs, rt
@@ -4811,6 +4811,14 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
  *
  *     (rd, LO, HI) <- rs * rt
  *
+ * and
+ *
+ *     MADD[U]    rd, rs, rt
+ *
+ * such that
+ *
+ *     (rd, LO, HI) <- (LO, HI) + rs * rt
+ *
  * where the low-order 32-bits of the result is placed into both the
  * GPR rd and the special register LO. The high-order 32-bits of the
  * result is placed into the special register HI.
@@ -4867,8 +4875,48 @@ static void gen_mul_txx9(DisasContext *ctx, uint32_t opc,
             tcg_temp_free_i32(t3);
         }
         break;
+    case TX79_MMI_MADD:
+        {
+            TCGv_i64 t2 = tcg_temp_new_i64();
+            TCGv_i64 t3 = tcg_temp_new_i64();
+
+            tcg_gen_ext_tl_i64(t2, t0);
+            tcg_gen_ext_tl_i64(t3, t1);
+            tcg_gen_mul_i64(t2, t2, t3);
+            tcg_gen_concat_tl_i64(t3, cpu_LO[acc], cpu_HI[acc]);
+            tcg_gen_add_i64(t2, t2, t3);
+            tcg_temp_free_i64(t3);
+            gen_move_low32(cpu_LO[acc], t2);
+            gen_move_high32(cpu_HI[acc], t2);
+            if (rd) {
+                gen_move_low32(cpu_gpr[rd], t2);
+            }
+            tcg_temp_free_i64(t2);
+        }
+        break;
+    case TX79_MMI_MADDU:
+        {
+            TCGv_i64 t2 = tcg_temp_new_i64();
+            TCGv_i64 t3 = tcg_temp_new_i64();
+
+            tcg_gen_ext32u_tl(t0, t0);
+            tcg_gen_ext32u_tl(t1, t1);
+            tcg_gen_extu_tl_i64(t2, t0);
+            tcg_gen_extu_tl_i64(t3, t1);
+            tcg_gen_mul_i64(t2, t2, t3);
+            tcg_gen_concat_tl_i64(t3, cpu_LO[acc], cpu_HI[acc]);
+            tcg_gen_add_i64(t2, t2, t3);
+            tcg_temp_free_i64(t3);
+            gen_move_low32(cpu_LO[acc], t2);
+            gen_move_high32(cpu_HI[acc], t2);
+            if (rd) {
+                gen_move_low32(cpu_gpr[rd], t2);
+            }
+            tcg_temp_free_i64(t2);
+        }
+        break;
     default:
-        MIPS_INVAL("mul TXx9");
+        MIPS_INVAL("mul/madd TXx9");
         generate_exception_end(ctx, EXCP_RI);
         goto out;
     }
@@ -24699,6 +24747,8 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx)
         break;
     case TX79_MMI_MULT1:
     case TX79_MMI_MULTU1:
+    case TX79_MMI_MADD:
+    case TX79_MMI_MADDU:
         gen_mul_txx9(ctx, opc, rd, rs, rt);
         break;
     case TX79_MMI_DIV1:
@@ -24713,8 +24763,6 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx)
     case TX79_MMI_MFHI1:
         gen_HILO(ctx, opc, 1, rd);
         break;
-    case TX79_MMI_MADD:          /* TODO: TX79_MMI_MADD */
-    case TX79_MMI_MADDU:         /* TODO: TX79_MMI_MADDU */
     case TX79_MMI_PLZCW:         /* TODO: TX79_MMI_PLZCW */
     case TX79_MMI_MADD1:         /* TODO: TX79_MMI_MADD1 */
     case TX79_MMI_MADDU1:        /* TODO: TX79_MMI_MADDU1 */

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Aleksandar Markovic]

> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, October 16, 2018 8:37 PM
> Subject: Re: [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU
> 
> On 10/16/18 11:19 AM, Fredrik Noring wrote:
> > /* global register indices */
> > static TCGv cpu_gpr[32], cpu_PC;
> > static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC];
> > 
> > One option is to create a new array such as
> > 
> > static TCGv_i64 mmi_gpr[32];
> > 
> > that represents the upper 64 bits of each GPR. Then cpu_gpr must be of
> > a 64-bit type too, even when QEMU runs in 32-bit user mode. The R5900
> > does not implement CP0.Status.UX in hardware, though, so system mode is
> > 64 bits, regardless.
> 
> I would not implement r5900 for mips32 in that case,
> I would implement it only for TARGET_MIPS64.
> 
> r~

Hello, Richard.

I truly need your help here. As you can conclude from the discussion, R5900 folks (anybody correct me if I am wrong) have some problems using any ABI other than O32. (For example, the standard gcc switch are "-mabi=32 -march=r5900".) Other similar CPUs, for example R4000, are built with TARGET_MIPS64, both user and system mode. R5900 would not have TARGET_MIPS64 in such arrangement. This looks outlandish to me. Given that R5900 is a 64-bit MIPS III-like processor, is there any anomaly in this arrangement, or this should work and is OK?

Thanks,
Aleksandar 

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Maciej W. Rozycki]

On Sun, 28 Oct 2018, Aleksandar Markovic wrote:

> I truly need your help here. As you can conclude from the discussion, 
> R5900 folks (anybody correct me if I am wrong) have some problems using 
> any ABI other than O32.

 The maximum the R5900 can support is the n32 ABI, owing to 32-bit virtual 
addressing.  And that ABI is even more troublesome due to this processor's 
peculiarities, requiring extra effort in addition to what has to be done 
to support o32 only.  The lack of sign-extension of the link address that 
becomes visible with 64-bit register accesses is just one issue to name.  
Hence the staged approach chosen.  We'll get n32 support eventually as 
well, once o32 has been sorted, but there'll be no n64 support ever.

  Maciej

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Aleksandar Markovic]

> 
> From: Aleksandar Markovic
> Subject: Re: [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU
> 
> > From: Richard Henderson <richard.henderson@linaro.org>
> > Sent: Tuesday, October 16, 2018 8:37 PM
> > Subject: Re: [PATCH] target/mips: Support Toshiba specific three-operand MADD and > MADDU
> >
> > On 10/16/18 11:19 AM, Fredrik Noring wrote:
> > > /* global register indices */
> > > static TCGv cpu_gpr[32], cpu_PC;
> > > static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC];
> > >
> > > One option is to create a new array such as
> > >
> > > static TCGv_i64 mmi_gpr[32];
> > >
> > > that represents the upper 64 bits of each GPR. Then cpu_gpr must be of
> > > a 64-bit type too, even when QEMU runs in 32-bit user mode. The R5900
> > > does not implement CP0.Status.UX in hardware, though, so system mode is
> > > 64 bits, regardless.
> >
> > I would not implement r5900 for mips32 in that case,
> > I would implement it only for TARGET_MIPS64.
> >
> > r~
> 
> Hello, Richard.
> 
> I truly need your help here. As you can conclude from the discussion, R5900 folks
> (anybody correct me if I am wrong) have some problems using any ABI other than O32.
> (For example, the standard gcc switch are "-mabi=32 -march=r5900".) Other similar CPUs,
> for example R4000, are built with TARGET_MIPS64, both user and system mode. R5900
> would not have TARGET_MIPS64 in such arrangement. This looks outlandish to me. Given
> that R5900 is a 64-bit MIPS III-like processor, is there any anomaly in this
> arrangement, or this should work and is OK?
> 
> Thanks,
> Aleksandar
> 

Guys,

Without TARGET_MIPS64, we can't say we emulate R5900 - we are emulating some other CPU that never existed.

Convince me that I am wrong.

Aleksandar

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Fredrik Noring]

Hi Aleksandar,

> Without TARGET_MIPS64, we can't say we emulate R5900 - we are emulating
> some other CPU that never existed.
> 
> Convince me that I am wrong.

R5900 O32 is usable. The R5900 toolchain is not yet ready for N32. Regarding
your proposal to rename TX79_MMI to MMI: what other ISAs do you have in mind
and do they also have 128-bit GPRs?

Fredrik

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: Re: [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU
> 
> Hi Aleksandar,
> 
> > Without TARGET_MIPS64, we can't say we emulate R5900 - we are emulating
> > some other CPU that never existed.
> >
> > Convince me that I am wrong.
> 
> R5900 O32 is usable.

Absolutely not. This kind of emulation infidelity can't be in QEMU upstream. Even one step further, this should be forbidden, impossible to build.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH] target/mips: Support Toshiba specific three-operand MADD and MADDU

[Maciej W. Rozycki]

On Mon, 29 Oct 2018, Aleksandar Markovic wrote:

> > > Without TARGET_MIPS64, we can't say we emulate R5900 - we are emulating
> > > some other CPU that never existed.
> > >
> > > Convince me that I am wrong.
> > 
> > R5900 O32 is usable.
> 
> Absolutely not. This kind of emulation infidelity can't be in QEMU 
> upstream. Even one step further, this should be forbidden, impossible to 
> build.

 What do you mean by "R5900 o32 is not usable unless TARGET_MIPS64"?

 The user emulation mode does not emulate a CPU, it emulates an OS's user 
ABI.  None of the MIPS/Linux user ABIs exposes the privileged context and 
the user instruction set is also defined by the ABI, as far as both 
limitations (e.g. in o32 no MIPS III instructions are allowed, though 
32-bit MIPS IV ones are) and extensions (e.g. many FPU instructions, LL/SC 
and RDHWR implemented even if missing from hardware) are concerned.

 So user emulation is never going to be an accurate CPU emulation anyway.

 The R5900 specifically indeed has no way to enforce the o32 instruction 
set, unlike all the other MIPS CPUs, but that has to be considered a 
quirk, because the operation of the disallowed instructions is 
unpredictable in o32 anyway and the first context switch when actually 
executing real Linux rather than QEMU user emulation would clobber user 
registers.  And I don't think we need to define unpredictable operation in 
hardware-specific way, it may well send SIGILL telling the user they did 
something wrong (as opposed to silent context corruption observed with 
real hw).

 If you spoke about system emulation, then that would be a different 
matter and I would tend to agree, noting however that even there QEMU does 
not emulate all aspects of the architecture anyway, so it's always an 
approximation defined such as to fit the people's needs.

 What is this discussion about anyway?  Do you require now for new CPU 
subarchitecture acceptance that the whole instruction set has been 
implemented with the first submission?  I don't think that was the case in 
the past and even artificial inexisting CPUs have been created (which 
still are there AFAICT), but maybe I'm missing something, and of course 
the rules may change.

  Maciej