Re: [Qemu-devel] [PATCH v2 22/27] tcg-ppc64: Use TCGType throughout compares

[Aurelien Jarno]

On Mon, Mar 04, 2013 at 04:33:05PM -0800, Richard Henderson wrote:
> The optimization/bug being fixed is that tcg_out_cmp was not applying the
> right type to loading a constant, in the case it can't be implemented
> directly.  Rather than recomputing the TCGType enum from the arch64 bool,
> pass around the original TCGType throughout.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  tcg/ppc64/tcg-target.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/tcg/ppc64/tcg-target.c b/tcg/ppc64/tcg-target.c
> index e34e548..355689a 100644
> --- a/tcg/ppc64/tcg-target.c
> +++ b/tcg/ppc64/tcg-target.c
> @@ -1053,8 +1053,8 @@ static void tcg_out_st (TCGContext *s, TCGType type, 
> TCGReg arg, TCGReg arg1,
>          tcg_out_ldsta (s, arg, arg1, arg2, STD, STDX);
>  }
>  
> -static void tcg_out_cmp (TCGContext *s, int cond, TCGArg arg1, TCGArg arg2,
> -                         int const_arg2, int cr, int arch64)
> +static void tcg_out_cmp(TCGContext *s, int cond, TCGArg arg1, TCGArg arg2,
> +                        int const_arg2, int cr, TCGType type)
>  {
>      int imm;
>      uint32_t op;
> @@ -1111,19 +1111,17 @@ static void tcg_out_cmp (TCGContext *s, int cond, 
> TCGArg arg1, TCGArg arg2,
>      default:
>          tcg_abort ();
>      }
> -    op |= BF (cr) | (arch64 << 21);
> +    op |= BF(cr) | ((type == TCG_TYPE_I64) << 21);
>  
> -    if (imm)
> -        tcg_out32 (s, op | RA (arg1) | (arg2 & 0xffff));
> -    else {
> +    if (imm) {
> +        tcg_out32(s, op | RA(arg1) | (arg2 & 0xffff));
> +    } else {
>          if (const_arg2) {
> -            tcg_out_movi (s, TCG_TYPE_I64, 0, arg2);
> -            tcg_out32 (s, op | RA (arg1) | RB (0));
> +            tcg_out_movi(s, type, 0, arg2);
> +            arg2 = 0;
>          }
> -        else
> -            tcg_out32 (s, op | RA (arg1) | RB (arg2));
> +        tcg_out32(s, op | RA(arg1) | RB(arg2));
>      }
> -
>  }

I do wonder if brcond should accept operand of the 'i' type, while in
practice it might only load 'I' or 'J' type.

That said your patch is already an improvement, so:

Reviewed-by: Aurelien Jarno <address@hidden>

>  static void tcg_out_setcond (TCGContext *s, TCGType type, TCGCond cond,
> @@ -1217,7 +1215,7 @@ static void tcg_out_setcond (TCGContext *s, TCGType 
> type, TCGCond cond,
>          sh = 31;
>          crop = CRNOR | BT (7, CR_EQ) | BA (7, CR_GT) | BB (7, CR_GT);
>      crtest:
> -        tcg_out_cmp (s, cond, arg1, arg2, const_arg2, 7, type == 
> TCG_TYPE_I64);
> +        tcg_out_cmp(s, cond, arg1, arg2, const_arg2, 7, type);
>          if (crop) tcg_out32 (s, crop);
>          tcg_out32 (s, MFCR | RT (0));
>          tcg_out_rlw(s, RLWINM, arg0, 0, sh, 31, 31);
> @@ -1243,12 +1241,12 @@ static void tcg_out_bc (TCGContext *s, int bc, int 
> label_index)
>      }
>  }
>  
> -static void tcg_out_brcond (TCGContext *s, TCGCond cond,
> -                            TCGArg arg1, TCGArg arg2, int const_arg2,
> -                            int label_index, int arch64)
> +static void tcg_out_brcond(TCGContext *s, TCGCond cond,
> +                           TCGArg arg1, TCGArg arg2, int const_arg2,
> +                           int label_index, TCGType type)
>  {
> -    tcg_out_cmp (s, cond, arg1, arg2, const_arg2, 7, arch64);
> -    tcg_out_bc (s, tcg_to_bc[cond], label_index);
> +    tcg_out_cmp(s, cond, arg1, arg2, const_arg2, 7, type);
> +    tcg_out_bc(s, tcg_to_bc[cond], label_index);
>  }
>  
>  void ppc_tb_set_jmp_target (unsigned long jmp_addr, unsigned long addr)
> @@ -1532,11 +1530,13 @@ static void tcg_out_op (TCGContext *s, TCGOpcode opc, 
> const TCGArg *args,
>          break;
>  
>      case INDEX_op_brcond_i32:
> -        tcg_out_brcond (s, args[2], args[0], args[1], const_args[1], 
> args[3], 0);
> +        tcg_out_brcond(s, args[2], args[0], args[1], const_args[1],
> +                       args[3], TCG_TYPE_I32);
>          break;
>  
>      case INDEX_op_brcond_i64:
> -        tcg_out_brcond (s, args[2], args[0], args[1], const_args[1], 
> args[3], 1);
> +        tcg_out_brcond(s, args[2], args[0], args[1], const_args[1],
> +                       args[3], TCG_TYPE_I64);
>          break;
>  
>      case INDEX_op_neg_i32:
> -- 
> 1.8.1.2
> 
> 
> 

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