Re: [Qemu-devel] [PATCH 06/11] target-mips: add gen_base_offset_addr

[Aurelien Jarno <aurelien@aurel32.net>]

On Mon, Nov 23, 2009 at 12:50:04PM -0800, Nathan Froyd wrote:
> This is a common pattern in existing code.  We'll also use it to
> implement the mips16 SAVE/RESTORE instructions.
> 
> Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
> ---
>  target-mips/translate.c |   40 ++++++++++++++++------------------------
>  1 files changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 1157e97..fece3c1 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -997,6 +997,19 @@ OP_ST_ATOMIC(scd,st64,ld64,0x7);
>  #endif
>  #undef OP_ST_ATOMIC
>  
> +static void gen_base_offset_addr (DisasContext *ctx, TCGv addr,
> +                                  int base, int16_t offset)
> +{
> +    if (base == 0) {
> +        tcg_gen_movi_tl(addr, offset);
> +    } else if (offset == 0) {
> +        gen_load_gpr(addr, base);
> +    } else {
> +        tcg_gen_movi_tl(addr, offset);
> +        gen_op_addr_add(ctx, addr, addr, cpu_gpr[base]);

I am nitpicking a bit, but it's better to keep the two last argument as
in the original version to keep the immediate value in the last
position. This saves one instruction on non-RISC hosts.

> +    }
> +}
> +
>  /* Load and store */
>  static void gen_ldst (DisasContext *ctx, uint32_t opc, int rt,
>                        int base, int16_t offset)
> @@ -1005,14 +1018,7 @@ static void gen_ldst (DisasContext *ctx, uint32_t opc, int rt,
>      TCGv t0 = tcg_temp_new();
>      TCGv t1 = tcg_temp_new();
>  
> -    if (base == 0) {
> -        tcg_gen_movi_tl(t0, offset);
> -    } else if (offset == 0) {
> -        gen_load_gpr(t0, base);
> -    } else {
> -        tcg_gen_movi_tl(t0, offset);
> -        gen_op_addr_add(ctx, t0, cpu_gpr[base], t0);
> -    }
> +    gen_base_offset_addr(ctx, t0, base, offset);
>      /* Don't do NOP if destination is zero: we must perform the actual
>         memory access. */
>      switch (opc) {
> @@ -1163,14 +1169,7 @@ static void gen_st_cond (DisasContext *ctx, uint32_t opc, int rt,
>  
>      t0 = tcg_temp_local_new();
>  
> -    if (base == 0) {
> -        tcg_gen_movi_tl(t0, offset);
> -    } else if (offset == 0) {
> -        gen_load_gpr(t0, base);
> -    } else {
> -        tcg_gen_movi_tl(t0, offset);
> -        gen_op_addr_add(ctx, t0, cpu_gpr[base], t0);
> -    }
> +    gen_base_offset_addr(ctx, t0, base, offset);
>      /* Don't do NOP if destination is zero: we must perform the actual
>         memory access. */
>  
> @@ -1202,14 +1201,7 @@ static void gen_flt_ldst (DisasContext *ctx, uint32_t opc, int ft,
>      const char *opn = "flt_ldst";
>      TCGv t0 = tcg_temp_new();
>  
> -    if (base == 0) {
> -        tcg_gen_movi_tl(t0, offset);
> -    } else if (offset == 0) {
> -        gen_load_gpr(t0, base);
> -    } else {
> -        tcg_gen_movi_tl(t0, offset);
> -        gen_op_addr_add(ctx, t0, cpu_gpr[base], t0);
> -    }
> +    gen_base_offset_addr(ctx, t0, base, offset);
>      /* Don't do NOP if destination is zero: we must perform the actual
>         memory access. */
>      switch (opc) {
> -- 
> 1.6.3.2
> 
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net