Patchwork target-mips: Sign-extend the result of LWR

login
register
mail settings
Submitter Richard Sandiford
Date Jan. 20, 2013, 7:30 p.m.
Message ID <87pq0zlki9.fsf@talisman.default>
Download mbox | patch
Permalink /patch/213984/
State New
Headers show

Comments

Richard Sandiford - Jan. 20, 2013, 7:30 p.m.
Sign-extend the result of LWR, as is already done for LWL.  This is necessary
in the case where LWR loads the full word (i.e. the address is actually
aligned).  In the other cases, it is implementation defined whether the
upper 32 bits of the result are unchanged or a copy of bit 31.  The latter
seems easier to implement.

Previously the code used:

    (oldval & (0xfffffffe << (31 - bitshift))) | (newval >> bitshift)

which zeroed the upper bits of the register, losing any previous sign
extension in the unaligned cases.

Signed-off-by: Richard Sandiford <rdsandiford@googlemail.com>
---
 target-mips/translate.c | 1 +
 1 file changed, 1 insertion(+)
Richard Henderson - Jan. 24, 2013, 4:35 p.m.
On 2013-01-20 11:30, Richard Sandiford wrote:
> Sign-extend the result of LWR, as is already done for LWL.  This is necessary
> in the case where LWR loads the full word (i.e. the address is actually
> aligned).  In the other cases, it is implementation defined whether the
> upper 32 bits of the result are unchanged or a copy of bit 31.  The latter
> seems easier to implement.
>
> Previously the code used:
>
>      (oldval & (0xfffffffe << (31 - bitshift))) | (newval >> bitshift)
>
> which zeroed the upper bits of the register, losing any previous sign
> extension in the unaligned cases.
>
> Signed-off-by: Richard Sandiford <rdsandiford@googlemail.com>
> ---
>   target-mips/translate.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <rth@twiddle.net>

r~
Aurelien Jarno - Jan. 31, 2013, 11 p.m.
On Sun, Jan 20, 2013 at 07:30:54PM +0000, Richard Sandiford wrote:
> Sign-extend the result of LWR, as is already done for LWL.  This is necessary
> in the case where LWR loads the full word (i.e. the address is actually
> aligned).  In the other cases, it is implementation defined whether the
> upper 32 bits of the result are unchanged or a copy of bit 31.  The latter
> seems easier to implement.
> 
> Previously the code used:
> 
>     (oldval & (0xfffffffe << (31 - bitshift))) | (newval >> bitshift)
> 
> which zeroed the upper bits of the register, losing any previous sign
> extension in the unaligned cases.
> 
> Signed-off-by: Richard Sandiford <rdsandiford@googlemail.com>
> ---
>  target-mips/translate.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 623edd0..08e28f3 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -1735,6 +1735,7 @@ static void gen_ld (CPUMIPSState *env, DisasContext *ctx, uint32_t opc,
>          tcg_temp_free(t2);
>          tcg_gen_or_tl(t0, t0, t1);
>          tcg_temp_free(t1);
> +        tcg_gen_ext32s_tl(t0, t0);
>          gen_store_gpr(t0, rt);
>          opn = "lwr";
>          break;
> -- 
> 1.7.11.7
> 
> 

Thanks, applied.

Patch

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 623edd0..08e28f3 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -1735,6 +1735,7 @@  static void gen_ld (CPUMIPSState *env, DisasContext *ctx, uint32_t opc,
         tcg_temp_free(t2);
         tcg_gen_or_tl(t0, t0, t1);
         tcg_temp_free(t1);
+        tcg_gen_ext32s_tl(t0, t0);
         gen_store_gpr(t0, rt);
         opn = "lwr";
         break;