Re: [Qemu-devel] [PATCH 14/60] AArch64: Add orr instruction emulation

[Claudio Fontana <claudio.fontana@linaro.org>]

Btw, in the first patch:

On 11/18/2013 02:12 PM, Michael Matz wrote:
> 
> From df54486da31d6329696effa61096eda5ab85395a Mon Sep 17 00:00:00 2001
> From: Michael Matz <matz@suse.de>
> Date: Sun, 24 Mar 2013 02:52:42 +0100
> Subject: [PATCH] Fix 32bit rotates.
> 
> The 32bit shifts generally weren't careful with the upper parts,
> either bits could leak in (for right shift) or leak or (for left shift).
> And rotate was completely off, rotating around bit 63, not 31.
> This fixes the CAST5 hash algorithm.
> ---
>  target-arm/translate-a64.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 96dc281..e3941a1 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -596,25 +596,49 @@ static TCGv_i64 get_shift(int reg, int shift_type, TCGv_i64 tcg_shift,
>      r = tcg_temp_new_i64();
>  
>      /* XXX carry_out */
> +    /* Careful with the width.  We work on 64bit, but must make sure
> +       that we zero-extend the result on out, and ignore any upper bits,
> +       that might still be set in that register.  */
>      switch (shift_type) {
>      case 0: /* LSL */
> +	/* left shift is easy, simply zero-extend on out */
>          tcg_gen_shl_i64(r, cpu_reg(reg), tcg_shift);
> +	if (is_32bit)
> +	  tcg_gen_ext32u_i64 (r, r);
>          break;
>      case 1: /* LSR */
> -        tcg_gen_shr_i64(r, cpu_reg(reg), tcg_shift);
> +	/* For logical right shift we zero extend first, to zero
> +	   the upper bits.  We don't need to extend on out.  */
> +	if (is_32bit) {
> +	    tcg_gen_ext32u_i64 (r, cpu_reg(reg));
> +	    tcg_gen_shr_i64 (r, r, tcg_shift);
> +	} else
> +	  tcg_gen_shr_i64(r, cpu_reg(reg), tcg_shift);
>          break;
>      case 2: /* ASR */
> +	/* For arithmetic right shift we sign extend first, then shift,
> +	   and then need to clear the upper bits again.  */
>          if (is_32bit) {
>              TCGv_i64 tcg_tmp = tcg_temp_new_i64();
>              tcg_gen_ext32s_i64(tcg_tmp, cpu_reg(reg));
>              tcg_gen_sar_i64(r, tcg_tmp, tcg_shift);
> +	    tcg_gen_ext32u_i64 (r, r);
>              tcg_temp_free_i64(tcg_tmp);
>          } else {
>              tcg_gen_sar_i64(r, cpu_reg(reg), tcg_shift);
>          }
>          break;
> -    case 3:
> -        tcg_gen_rotr_i64(r, cpu_reg(reg), tcg_shift);
> +    case 3: /* ROR */
> +	/* For rotation extending doesn't help, we really have to use
> +	   a 32bit rotate.  */
> +	if (is_32bit) {
> +	    TCGv_i32 tmp = tcg_temp_new_i32();
> +            tcg_gen_trunc_i64_i32(tmp, cpu_reg(reg));
> +	    tcg_gen_rotr_i32(tmp, tmp, tcg_shift);

Isn't this problematic?
We are using gen_rotr_i32, but passing tcg_shift, which is a TCGv_i64.
I remember I had compilation failures in the past when I tried something similar,
so my understanding is that this can work with a certain compiler under certain compiler options,
but is not guaranteed to work in all cases.

I think we need to either explicitly convert the tcg_shift to a TCGv_i32, or we need to use an open coded version of the rotr_i64 that inserts at (32 - n) instead of (64 - n)

What do you think?

C.

> +            tcg_gen_extu_i32_i64(r, tmp);
> +            tcg_temp_free_i32(tmp);
> +	} else
> +	  tcg_gen_rotr_i64(r, cpu_reg(reg), tcg_shift);
>          break;
>      }
>  
> -- 1.8.1.4
>