Re: [PATCH 20/27] arcv3: TCG, decoder glue code and helper changes

[Richard Henderson <richard.henderson@linaro.org>]

On 4/5/21 7:31 AM, cupertinomiranda@gmail.com wrote:
> +uint64_t helper_carry_add_flag32(uint64_t dest, uint64_t b, uint64_t c) {
> +    return carry_add_flag(dest, b, c, 32);
> +}
> +
> +target_ulong helper_overflow_add_flag32(target_ulong dest, target_ulong b, target_ulong c) {
> +    return overflow_add_flag(dest, b, c, 32);
> +}
> +
> +target_ulong helper_overflow_sub_flag32(target_ulong dest, target_ulong b, target_ulong c) {
> +    dest = dest & 0xffffffff;
> +    b = b & 0xffffffff;
> +    c = c & 0xffffffff;
> +    return overflow_sub_flag(dest, b, c, 32);
> +}

You shouldn't need to replicate these functions.  Use the correct types and 
masking in the first place.


> +uint64_t helper_rotate_left32(uint64_t orig, uint64_t n)
> +{
> +    uint64_t t;
> +    uint64_t dest = (orig << n) & ((0xffffffff << n) & 0xffffffff);
> +
> +    t = (orig >> (32 - n)) & ((1 << n) - 1);
> +    dest |= t;
> +
> +    return dest;
> +}
> +
> +uint64_t helper_rotate_right32(uint64_t orig, uint64_t n)
> +{
> +    uint64_t t;
> +    uint64_t dest = (orig >> n) & (0xffffffff >> n);
> +
> +    t = ((orig & ((1 << n) - 1)) << (32 - n));
> +    dest |= t;
> +
> +    return dest;
> +}

rol32 and ror32.

> +uint64_t helper_asr_32(uint64_t b, uint64_t c)
> +{
> +  uint64_t t;
> +  c = c & 31;
> +  t = b;
> +  for(int i = 0; i < c; i++) {
> +    t >>= 1;
> +    if((b & 0x80000000) != 0)
> +      t |= 0x80000000;
> +  }
> +      //t |= ((1 << (c+1)) - 1) << (32 - c);
> +
> +  return t;

Really?  I can't imagine what lead you to write this.
Who writes a simple shift operation with a loop?

Perhaps no helper at all and

   tcg_gen_sra_tl(ret, b, c);
   tcg_gen_ext32s_tl(ret, ret);


> +target_ulong helper_ffs32(CPUARCState *env, uint64_t src)
> +{
> +    int i;
> +    if (src == 0) {
> +      return 31;
> +    }
> +    for (i = 0; i <= 31; i++) {
> +      if (((src >> i) & 1) != 0) {
> +        break;
> +      }
> +    }
> +    return i;
> +}

tcg_gen_ori_tl(ret, src, MAKE_64BIT_MASK(32, 32));
tcg_gen_ctzi_tl(ret, ret, 31);

Though I really wonder if you've got that function correct, as it's not the 
*normal* definition of ffs...


> +target_ulong helper_norml(CPUARCState *env, uint64_t src1)
> +{
> +    int i;
> +    int64_t tmp = (int64_t) src1;
> +    if (tmp == 0 || tmp == -1) {
> +      return 0;
> +    }
> +    for (i = 0; i <= 63; i++) {
> +      if ((tmp >> i) == 0) {
> +          break;
> +      }
> +      if ((tmp >> i) == -1) {
> +          break;
> +      }
> +    }
> +    return i;
> +}

This is some cognate of count-leading-repititions-of-sign-bit, 
tcg_gen_clrsb_tl.  A decent computation should be like

   tcg_gen_clrsb_i64(ret, src);
   tcg_gen_subfi_i64(ret, 63, ret);


> diff --git a/target/arc/semfunc-v2_mapping.def b/target/arc/semfunc-v2_mapping.def
> new file mode 100644
> index 0000000000..ab8d9ff123
> --- /dev/null
> +++ b/target/arc/semfunc-v2_mapping.def

You could have named this properly to start.


r~