Re: [Qemu-devel] [PATCH v4 5/5] target/mips: Refactor and fix INSERT.<B|H|W|D> instructions

[Aleksandar Markovic]

> From: Mateja Marjanovic <address@hidden>
> Subject: Re: [PATCH v4 5/5] target/mips: Refactor and fix INSERT.<B|H|W|D> 
> instructions
> 
> On 2.4.19. 22:50, Aleksandar Markovic wrote:
> >> From: Mateja Marjanovic <address@hidden>
> >> Subject: [PATCH v4 5/5] target/mips: Refactor and fix INSERT.<B|H|W|D> 
> >> instructions
> >>
> >> From: Mateja Marjanovic <address@hidden>
> >>
> >> The old version of the helper for the INSERT.<B|H|W|D> MSA instructions
> >> has been replaced with four helpers that don't use switch, and change
> >> the endianness of the given index, when executed on a big endian host.
> >>
> >> Signed-off-by: Mateja Marjanovic <address@hidden>
> >> ---
> > ...
> >
> >
> >> +    n %= 16;
> > Mateja, could you just clarify what is the purpose of this line (and
> > similar three lines involving "%=")? It looks to me that n is already
> > limited here to be between 0 and 15, isn't it? (that follows from the
> > source code of gen_msa_elm().) What made you insert this line,
> > as it stands?
> 
> It was
> 
> n %= DF_ELEMENTS(df);
> 
> but when I deleted the df argument, so it had to be done like
> this. I think it's a matter of precaution (in case a number
> greater than 15, or 8... is passed as an argument).

Oh, I see.
 
At this moment, I think a more appropriate code would be:

assert(n < 16)

The whole set of functions decoding this group of instructions is
too complicated (several layers, repeated calculations), and could
be further simplified. But let's say that would be outside of the
scope of this patch and series. Your series, in fact, begins that
simplification by removing redundant decoding of "df" in helpers,
so it is a good step towards better code organization, and we'll
leave more detailed cleanup for some future endeavors.

Yours,
Aleksandar