[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] target/mips: Fix insert.<b|h|w> for MIPS bi
From: |
Aleksandar Markovic |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] target/mips: Fix insert.<b|h|w> for MIPS big endian host |
Date: |
Fri, 22 Mar 2019 17:16:26 +0000 |
> From: Mateja Marjanovic <address@hidden>
> Subject: [PATCH 4/4] target/mips: Fix insert.<b|h|w> for MIPS big endian host
>
> From: Mateja Marjanovic <address@hidden>
>
> Inserting from GPR to an element in a MSA register when
> executed on a MIPS big endian CPU, didn't pick the
> right element, and was behaving like on little endian.
>
> Signed-off-by: Mateja Marjanovic <address@hidden>
> ---
...
> @@ -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>"
)?
Thanks,
Aleksandar