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 <m...@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 >