Re: [Qemu-devel] [PATCH 4/4] target/mips: Fix insert.<b|h|w> for MIPS big endian host

[Mateja Marjanovic]

On 22.3.19. 19:04, Aleksandar Markovic wrote:
> > > > @@ -1511,9 +1518,11 @@ void helper_msa_insert_df(CPUMIPSState *env, uint32_t 
> > > > df, uint32_t wd,
> > > >        case DF_WORD:
> > > >            pwd->w[n] = (int32_t)rs;
> > > >            break;
> > > > +#ifdef TARGET_MIPS64
> > > >        case DF_DOUBLE:
> > > >            pwd->d[n] = (int64_t)rs;
> > > >            break;
> > > > +#endif
> > > >        default:
> > > >            assert(0);
> > > >        }
> > > You are right that this case should be under ifdef the way you did.
> > >
> > > In fact, this code should be impossible to reach, since there is a check
> > > for MIPS32/64 in translate.c before invoking this helper, so technically
> > > there is no bug. However, it is a latent bag, and also an instance of
> > > "dead code" (for MIPS32). So, you are rightfully removing this case
> > > for MIPS32.
> > >
> > > May I just ask you to put this in a separate patch, since this has nothing
> > > to do with endianess etc. (with the title, let's say:
> > >
> > > "target/mips: Remove handling of nonexistent flavor of INSERT for MIPS32",
> > >
> > > and the commit message
> > >
> > > "INSERT.D is present in MIPS64 MSA only. [1] page <XXX>
> > >
> > > [1] <insert here the latest MSA MIPS64 doc>"
> > >
> > > )?
> > You are right, it has nothing to do with the endianness problem. I will
> > remove that in v2, and add another patch for that.
>
> Mateja, I noticed that FILL.D has almost identical problem.
>
> Could you create a separate patch for FILL.D too, dealing with the same
> issue?
>
> And also analyze all MIPS64-only MSA instructions in this regard?
>
> Glancing at the MIPS64 MSA doc, they are:
>
> COPY_S.D
> COPY_U.W (this is not a typo, it is W, not D)
> FILL.D
> INSERT.D
Will do.
> Yours,
> Aleksandar
Regards,
Mateja
> > > Thanks,
> > > Aleksandar
> > Thanks,
> > Mateja