Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
On Monday, January 14, 2019, Fredrik Noring wrote: > Hi Aleksandar, > > > Awesome! > > > > I am especially happy with your choice of naming "mmr" (MultiMedia > > Registers) for these fieilds, since that is what they really are (and > they > > are certainly not "gprs"). Right on the money! > > Great, thanks! > > > > For HI1 and LO1 only? I'm asking since HI0 and LO0 are implemented with > > > the DSP array anyway, for all ISAs. > > > > I leave it to your judgement. If you are not sure (or you find the > current > > implementation too sensitive or contrieved to touch), you can leave > HI1/LO1 > > fields implementation as it is now. My motivation was avoiding usage of > the > > same data fields for two relatively independant purposes. > > I think the change is simple, but what should we call the new variables? > > /* global register indices */ > static TCGv cpu_gpr[32], cpu_PC; > static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC]; > static TCGv cpu_HI1, cpu_LO1;/* Upper half of 128-bit TX79 HI and LO */ > > Something like the last line? > > Correct. > By the way, what are your thoughts on "[PATCH v2 3/6] target/mips: Fix > HI[ac] and LO[ac] 32-bit truncation with MIPS64 DSP ASE"? > > https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg01287.html > > Still taking a closer look. Didn't forget. > > Outstanding! I salute your including PCPYUD and PCPYLD in this group - > they > > too can be considered "basic R/W access to mmr". > > Good, many thanks! > > > The goal right now is to prepare basic stuff related to SA register, even > > though there is possibly no immediate any application use case. However, > > this will make potential future development considerably easier, so > please > > include handling of this register and these instructions. > > Done, although I have some minor clean-ups left to do. I have checked with > R5900 hardware to match the implementation defined value of the SA > register. > > I will post MFSA, MTSA, MTSAB and MTSAH in v2 of this patch series. > > Magnificent! > > Regarding segments: > > > > +int rs = extract32(ctx->opcode, 21, 5); > > +int rt = extract32(ctx->opcode, 16, 5); > > +int rd = extract32(ctx->opcode, 11, 5); > > > > Please include them in gen_XXX() functions, rather than in decode_XXX() > > functions. This will leave decode_XXX() functions with a single > > responsibility of detecting what instruction is about to be processed, > > which is cleaner from logical decomposition point of view (even if it > would > > sometimes result in the repetition of some code segments - logical > > decomposition is of far greater importance). > > Done! > > Fredrik >
Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
Hi Aleksandar, > Awesome! > > I am especially happy with your choice of naming "mmr" (MultiMedia > Registers) for these fieilds, since that is what they really are (and they > are certainly not "gprs"). Right on the money! Great, thanks! > > For HI1 and LO1 only? I'm asking since HI0 and LO0 are implemented with > > the DSP array anyway, for all ISAs. > > I leave it to your judgement. If you are not sure (or you find the current > implementation too sensitive or contrieved to touch), you can leave HI1/LO1 > fields implementation as it is now. My motivation was avoiding usage of the > same data fields for two relatively independant purposes. I think the change is simple, but what should we call the new variables? /* global register indices */ static TCGv cpu_gpr[32], cpu_PC; static TCGv cpu_HI[MIPS_DSP_ACC], cpu_LO[MIPS_DSP_ACC]; static TCGv cpu_HI1, cpu_LO1;/* Upper half of 128-bit TX79 HI and LO */ Something like the last line? By the way, what are your thoughts on "[PATCH v2 3/6] target/mips: Fix HI[ac] and LO[ac] 32-bit truncation with MIPS64 DSP ASE"? https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg01287.html > Outstanding! I salute your including PCPYUD and PCPYLD in this group - they > too can be considered "basic R/W access to mmr". Good, many thanks! > The goal right now is to prepare basic stuff related to SA register, even > though there is possibly no immediate any application use case. However, > this will make potential future development considerably easier, so please > include handling of this register and these instructions. Done, although I have some minor clean-ups left to do. I have checked with R5900 hardware to match the implementation defined value of the SA register. I will post MFSA, MTSA, MTSAB and MTSAH in v2 of this patch series. > Regarding segments: > > +int rs = extract32(ctx->opcode, 21, 5); > +int rt = extract32(ctx->opcode, 16, 5); > +int rd = extract32(ctx->opcode, 11, 5); > > Please include them in gen_XXX() functions, rather than in decode_XXX() > functions. This will leave decode_XXX() functions with a single > responsibility of detecting what instruction is about to be processed, > which is cleaner from logical decomposition point of view (even if it would > sometimes result in the repetition of some code segments - logical > decomposition is of far greater importance). Done! Fredrik
Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
On Sunday, January 13, 2019, Fredrik Noring wrote: > Hi Aleksandar, > > > - Suggestion: The next MIPS pull request is scehuled for Friday, > > Jan 18, 2018. It would be fantastic if you could prepare the > > following by Jan 14: > > > > * Add 32 TCGv_i64 registers that would represent higher halves > > of R5900 general purpose registers. > > Done! Awesome! I am especially happy with your choice of naming "mmr" (MultiMedia Registers) for these fieilds, since that is what they really are (and they are certainly not "gprs"). Right on the money! > > > * Add TCGv_i32 register SA (shift amount). > > See notes below. > > > * Perhaps consider adding higher halves of registers HI an LO > > independently on HI/LO array used by DSP. > > For HI1 and LO1 only? I'm asking since HI0 and LO0 are implemented with > the DSP array anyway, for all ISAs. > > I leave it to your judgement. If you are not sure (or you find the current implementation too sensitive or contrieved to touch), you can leave HI1/LO1 fields implementation as it is now. My motivation was avoiding usage of the same data fields for two relatively independant purposes. > > * It is customary to implement R/W access while introducing > > such registers: > > * Implement R/W access instructions to higher halves of > > R5900 GPRs: > > * LQ > > Done, including PCPYUD and PCPYLD for proper testing! > > Outstanding! I salute your including PCPYUD and PCPYLD in this group - they too can be considered "basic R/W access to mmr". > > * SQ > > Done, including testing! > > Perfect! > > * Implement R/W access instructions to SA register: > > * MFSA > > The TX79 manual says that "the sole purpose of this instruction is to > permit the shift amount to be saved during a context switch" and that > "the shift amount is encoded in SA in an implementation-defined manner" > so it seems to make more sense for system mode rather than user mode? > > One may want to choose an implementation that matches the actual R5900 > hardware, even though the manual says it's arbitrary. > > > * MTSA > > Likewise. > > > * MTSAH > > * MTSAB > > These instructions do not appear to be usable unless the corresponding > shift instructions are implemented as well? > > The goal right now is to prepare basic stuff related to SA register, even though there is possibly no immediate any application use case. However, this will make potential future development considerably easier, so please include handling of this register and these instructions. > I will post an R5900 multimedia instruction patch series shortly. > > Yes, go ahead, please! I unfortunatelly will not have enough time for detailed review before Tuesday, but I have a relatively simple hint right now: Regarding segments: +int rs = extract32(ctx->opcode, 21, 5); +int rt = extract32(ctx->opcode, 16, 5); +int rd = extract32(ctx->opcode, 11, 5); Please include them in gen_XXX() functions, rather than in decode_XXX() functions. This will leave decode_XXX() functions with a single responsibility of detecting what instruction is about to be processed, which is cleaner from logical decomposition point of view (even if it would sometimes result in the repetition of some code segments - logical decomposition is of far greater importance). Thanks for all efforts!! Aleksandar > Fredrik > >
Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
Hi Aleksandar, > - Suggestion: The next MIPS pull request is scehuled for Friday, > Jan 18, 2018. It would be fantastic if you could prepare the > following by Jan 14: > > * Add 32 TCGv_i64 registers that would represent higher halves > of R5900 general purpose registers. Done! > * Add TCGv_i32 register SA (shift amount). See notes below. > * Perhaps consider adding higher halves of registers HI an LO > independently on HI/LO array used by DSP. For HI1 and LO1 only? I'm asking since HI0 and LO0 are implemented with the DSP array anyway, for all ISAs. > * It is customary to implement R/W access while introducing > such registers: > * Implement R/W access instructions to higher halves of > R5900 GPRs: > * LQ Done, including PCPYUD and PCPYLD for proper testing! > * SQ Done, including testing! > * Implement R/W access instructions to SA register: > * MFSA The TX79 manual says that "the sole purpose of this instruction is to permit the shift amount to be saved during a context switch" and that "the shift amount is encoded in SA in an implementation-defined manner" so it seems to make more sense for system mode rather than user mode? One may want to choose an implementation that matches the actual R5900 hardware, even though the manual says it's arbitrary. > * MTSA Likewise. > * MTSAH > * MTSAB These instructions do not appear to be usable unless the corresponding shift instructions are implemented as well? I will post an R5900 multimedia instruction patch series shortly. Fredrik
Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
Hi Aleksandar,
> Glad to see you back!
Likewise!
> Yes, one can say this is a step towards reenabling R5900 support.
Great!
> At this moment I have a question a suggestion for you:
>
> - Question: Do you have somewhere link to n32 R5900 toolchain, or
> similar thing that would enable me to test R5900's n32 in QEMU
Yes. The only (small) changes needed for n32 are related to Glibc,
since the R5900 does not implement DMULT etc. in hardware. The attached
patch (see below) traps these instructions, but the Glibc patch proposal
(not yet submitted) will instead emulate them, which is believed to be
faster (with actual hardware; with QEMU it would most likely instead be
significantly slower since QEMU would need to emulate this emulation).
You will also need commit d728eb9085d8 ("MIPS: Default to --with-llsc for
the R5900 Linux target as well"), made for o32 and GCC, as explained in
https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03649.html
unless you compile GCC from HEAD that already has it.
> (I'll do the tweaks in QEMU by myself for that, but I need a way
> to compile R5900 test programs for n32)?
I just verified that the only change needed with QEMU apart from reverting
commit 823f2897bdd7 ("target/mips: Disable R5900 support") is this:
--- a/linux-user/mips64/target_elf.h
+++ b/linux-user/mips64/target_elf.h
@@ -12,6 +12,9 @@ static inline const char *cpu_get_model(uint32_t eflags)
if ((eflags & EF_MIPS_ARCH) == EF_MIPS_ARCH_64R6) {
return "I6400";
}
+if ((eflags & EF_MIPS_MACH) == EF_MIPS_MACH_5900) {
+return "R5900";
+}
return "5KEf";
}
#endif
> - Suggestion: The next MIPS pull request is scehuled for Friday,
> Jan 18, 2018. It would be fantastic if you could prepare the
> following by Jan 14:
>
> * Add 32 TCGv_i64 registers that would represent higher halves
> of R5900 general purpose registers.
> * Add TCGv_i32 register SA (shift amount).
> * Perhaps consider adding higher halves of registers HI an LO
> independently on HI/LO array used by DSP.
> * It is customary to implement R/W access while introducing
> such registers:
> * Implement R/W access instructions to higher halves of
> R5900 GPRs:
> * LQ
> * SQ
> * Implement R/W access instructions to SA register:
> * MFSA
> * MTSA
> * MTSAH
> * MTSAB
Sounds good except I'm not sure it can be done before 14 January!
> I think a reasonable person would consider that the number and
> size of registers of emulated CPU is a fundamental thing that
> must be done before enabling its support - hence my suggestion
> above.
Well, no package in any popular Linux distribution require these R5900
extensions, or as far as I know, try to make use of them. So with an
initial use case of running a Linux distribution in user mode, support
for them is certainly not needed at all.
Of course, support would be needed if anyone would like to start
implementing R5900 specific extensions. Also, the Linux kernel needs a
few of those instructions, so implementing them would make it possible
to run the kernel in system mode, which would be very useful too.
> If you don't have enough time resources before Jan 14, that is
> fine too, do everything at your own pace.
Thanks!
> My vision is that we continue step by step amending R5900 until
> we reach a decent and stable state of emulation and than enable
> the R5900 support for end user. (I'll provide all the details
> later on.)
OK.
Fredrik
--- a/sysdeps/mips/mips64/addmul_1.S
+++ b/sysdeps/mips/mips64/addmul_1.S
@@ -33,6 +33,9 @@
.option pic2
#endif
ENTRY (__mpn_addmul_1)
+#ifdef _MIPS_ARCH_R5900
+ .setmips3
+#endif
#ifdef PIC
SETUP_GP /* ??? unused */
#endif
diff --git a/sysdeps/mips/mips64/mul_1.S b/sysdeps/mips/mips64/mul_1.S
index 8707257a68..19e3ae5eae 100644
--- a/sysdeps/mips/mips64/mul_1.S
+++ b/sysdeps/mips/mips64/mul_1.S
@@ -34,6 +34,9 @@
.option pic2
#endif
ENTRY (__mpn_mul_1)
+#ifdef _MIPS_ARCH_R5900
+ .setmips3
+#endif
#ifdef __PIC__
SETUP_GP /* ??? unused */
#endif
diff --git a/sysdeps/mips/mips64/submul_1.S b/sysdeps/mips/mips64/submul_1.S
index fb9a1c2375..763c5bb687 100644
--- a/sysdeps/mips/mips64/submul_1.S
+++ b/sysdeps/mips/mips64/submul_1.S
@@ -34,6 +34,9 @@
.option pic2
#endif
ENTRY (__mpn_submul_1)
+#ifdef _MIPS_ARCH_R5900
+ .setmips3
+#endif
#ifdef __PIC__
SETUP_GP /* ??? unused */
#endif
diff --git a/sysdeps/mips/sys/tas.h b/sysdeps/mips/sys/tas.h
index d5ed013e28..ad797bfb1d 100644
--- a/sysdeps/mips/sys/tas.h
+++ b/sysdeps/mips/sys/tas.h
@@ -38,11 +38,14 @@ __NTH (_test_and_set (int *__p, int __v))
{
int __r, __t;
+ /* The R5900 reports itself as MIPS III but it does not have LL/SC. */
__asm__ __volatile__
("/* Inline test and set */\n"
".set push\n\t"
#if _MIPS_SIM == _ABIO32 && __mips < 2
".set mips2\n\t"
+#elif defined (_MIPS_ARCH_R5900)
+
Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
> From: Fredrik Noring
> Sent: Tuesday, January 1, 2019 7:27 PM
> To: Aleksandar Markovic
> Cc: Aurelien Jarno; Philippe Mathieu-Daudé; Jürgen Urban; Maciej W. Rozycki;
> qemu-devel@nongnu.org
> Subject: Re: [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
>
> Thanks Aleksandar!
>
> > > From: Fredrik Noring
> > > Subject: [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
> >
> > Reviewed-by: Aleksandar Markovic
> >
> > This patch is selected for integration in the next MIPS pull request
> > scheduled shortly.
> >
> > THANKS FOR ALL EFFORTS!
>
> Is this a preparation to revert commit 823f2897bdd7 ("target/mips: Disable
> R5900 support")?
>
> Fredrik
Hello!
Glad to see you back!
Yes, one can say this is a step towards reenabling R5900 support.
However, this is not the only step. I will let you know about all
needed details in near future - and I apologize for being little
slow with responses so far, I was being swamped with other tasks.
Let's not rush. If one learned that the rush is bad, that is me,
making severe missteps, overly eager to do the thing sooner rather
than later. However, the faster in not always the better.
At this moment I have a question a suggestion for you:
- Question: Do you have somewhere link to n32 R5900 toolchain, or
similar thing that would enable me to test R5900's n32 in QEMU
(I'll do the tweaks in QEMU by myself for that, but I need a way
to compile R5900 test programs for n32)?
- Suggestion: The next MIPS pull request is scehuled for Friday,
Jan 18, 2018. It would be fantastic if you could prepare the
following by Jan 14:
* Add 32 TCGv_i64 registers that would represent higher halves
of R5900 general purpose registers.
* Add TCGv_i32 register SA (shift amount).
* Perhaps consider adding higher halves of registers HI an LO
independently on HI/LO array used by DSP.
* It is customary to implement R/W access while introducing
such registers:
* Implement R/W access instructions to higher halves of
R5900 GPRs:
* LQ
* SQ
* Implement R/W access instructions to SA register:
* MFSA
* MTSA
* MTSAH
* MTSAB
I think a reasonable person would consider that the number and
size of registers of emulated CPU is a fundamental thing that
must be done before enabling its support - hence my suggestion
above.
If you don't have enough time resources before Jan 14, that is
fine too, do everything at your own pace.
My vision is that we continue step by step amending R5900 until
we reach a decent and stable state of emulation and than enable
the R5900 support for end user. (I'll provide all the details
later on.)
Let me know if you have questions and/or different opinion.
Looking forward to seeing you!
Aleksandar
Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
Thanks Aleksandar!
> > From: Fredrik Noring
> > Subject: [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
>
> Reviewed-by: Aleksandar Markovic
>
> This patch is selected for integration in the next MIPS pull request
> scheduled shortly.
>
> THANKS FOR ALL EFFORTS!
Is this a preparation to revert commit 823f2897bdd7 ("target/mips: Disable
R5900 support")?
Fredrik
Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
> From: Fredrik Noring > Subject: [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1 Reviewed-by: Aleksandar Markovic This patch is selected for integration in the next MIPS pull request scheduled shortly. THANKS FOR ALL EFFORTS!
[Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1
Signed-off-by: Fredrik Noring
---
tests/tcg/mips/mipsr5900/maddu.c | 37 ++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/tests/tcg/mips/mipsr5900/maddu.c b/tests/tcg/mips/mipsr5900/maddu.c
index e4e552102d..30936fb2b4 100644
--- a/tests/tcg/mips/mipsr5900/maddu.c
+++ b/tests/tcg/mips/mipsr5900/maddu.c
@@ -1,5 +1,5 @@
/*
- * Test R5900-specific three-operand MADDU.
+ * Test R5900-specific three-operand MADDU and MADDU1.
*/
#include
@@ -29,9 +29,42 @@ uint64_t maddu(uint64_t a, uint32_t rs, uint32_t rt)
return r;
}
+uint64_t maddu1(uint64_t a, uint32_t rs, uint32_t rt)
+{
+uint32_t lo = a;
+uint32_t hi = a >> 32;
+uint32_t rd;
+uint64_t r;
+
+__asm__ __volatile__ (
+"mtlo1 %5\n"
+"mthi1 %6\n"
+"maddu1 %0, %3, %4\n"
+"mflo1 %1\n"
+"mfhi1 %2\n"
+: "=r" (rd), "=r" (lo), "=r" (hi)
+: "r" (rs), "r" (rt), "r" (lo), "r" (hi));
+r = ((uint64_t)hi << 32) | (uint32_t)lo;
+
+assert(a + (uint64_t)rs * rt == r);
+assert(rd == lo);
+
+return r;
+}
+
+static int64_t maddu_variants(int64_t a, int32_t rs, int32_t rt)
+{
+int64_t rd = maddu(a, rs, rt);
+int64_t rd1 = maddu1(a, rs, rt);
+
+assert(rd == rd1);
+
+return rd;
+}
+
int main()
{
-assert(maddu(13, 17, 19) == 336);
+assert(maddu_variants(13, 17, 19) == 336);
return 0;
}
--
2.18.1
