On 11/18/20 12:29 AM, frank.ch...@sifive.com wrote: > +static target_ulong do_grev(target_ulong rs1, > + target_ulong rs2, > + const target_ulong masks[]) > +{
I think the masks should be placed here, and not passed in. What you should pass in is "int bits". > + target_ulong x = rs1; > + int shift = 1; > + int i = 0; > + > + while (shift < TARGET_LONG_BITS) { > + if (rs2 & shift) { > + x = do_swap(x, masks[i], shift); > + } > + shift <<= 1; > + ++i; Cleaner as for loop: for (i = 0, shift = 1; shift < bits; i++, shift <<= 1) > + static const target_ulong masks[] = { > +#ifdef TARGET_RISCV32 > + 0x55555555, 0x33333333, 0x0f0f0f0f, > + 0x00ff00ff, 0x0000ffff, > +#else > + 0x5555555555555555, 0x3333333333333333, > + 0x0f0f0f0f0f0f0f0f, 0x00ff00ff00ff00ff, > + 0x0000ffff0000ffff, 0x00000000ffffffff, > +#endif You don't need to replicate every entry. dup_const(0x55, MO_8), dup_const(0x33, MO_8), dup_const(0x0f, MO_8), dup_const(0xff, MO_16), dup_const(0xffff, MO_32), #ifdef TARGET_RISCV64 UINT32_MAX #endif > +target_ulong HELPER(grevw)(target_ulong rs1, target_ulong rs2) > +{ > + static const target_ulong masks[] = { > + 0x55555555, 0x33333333, 0x0f0f0f0f, > + 0x00ff00ff, 0x0000ffff, > + }; > + > + return do_grev(rs1, rs2, masks); This one is broken because do_grev iterated to TARGET_LONG_BITS == 64, and the masks array is too small. Fixed by passing in 32 as bits parameter to do_grev, as above. > +static bool trans_grevi(DisasContext *ctx, arg_grevi *a) > +{ > + REQUIRE_EXT(ctx, RVB); > + > + if (a->shamt >= TARGET_LONG_BITS) { > + return false; > + } > + > + return gen_grevi(ctx, a); > +} While this is ok for an initial implementation, it is worth noticing the shamt for rev8 as a special case for tcg_gen_bswap_tl. Otherwise, this needs the same gen_shift treatment. r~