Re: [Qemu-devel] [PATCH v2 09/12] tests/tcg/mips: Test R5900 three-operand MADDU1

[Aleksandar Markovic]

On Monday, January 14, 2019, Fredrik Noring <address@hidden> 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
>