Re: [PATCH v4 29/31] target/ppc: Implement cfuged instruction

[Richard Henderson]

On 5/12/21 1:54 PM, matheus.ferst@eldorado.org.br wrote:
> +    while (i) {
> +        n = ctz64(mask);
> +        if (n > i) {
> +            n = i;
> +        }
> +
> +        m = (1ll << n) - 1;
> +        if (bit) {
> +            right = ror64(right | (src & m), n);
> +        } else {
> +            left = ror64(left | (src & m), n);
> +        }
> +
> +        src >>= n;
> +        mask >>= n;
> +        i -= n;
> +        bit = !bit;
> +        mask = ~mask;
> +    }
> +
> +    if (bit) {
> +        n = ctpop64(mask);
> +    } else {
> +        n = 64 - ctpop64(mask);
> +    }
> +
> +    return left | (right >> n);
> +}

This doesn't correspond to the algorithm presented in the manual.  Thus this 
requires lots of extra commentary.

I guess I see how you're trying to process blocks at a time, instead of single 
bits at a time.  But I don't think the merging of data into "right" and "left" 
looks right.  I would have expected

    right = (right << n) | (src & m);

and similarly for left.

It doesn't look like that the ctpop at the end is correct, given how mask has 
been modified.  I would have thought that

    n = ctpop64(orig_mask);
    return (left << n) | right;

would be the correct answer.

I could be wrong about the above, but that's what the missing commentary should 
have helped me understand.

> +static bool trans_CFUGED(DisasContext *ctx, arg_X *a)
> +{
> +    REQUIRE_64BIT(ctx);
> +    REQUIRE_INSNS_FLAGS2(ctx, ISA310);
> +#if defined(TARGET_PPC64)
> +    gen_helper_cfuged(cpu_gpr[a->ra], cpu_gpr[a->rt], cpu_gpr[a->rb]);
> +#else
> +    gen_invalid(ctx);
> +#endif
> +    return true;
> +}

Given that this helper will also be used by vcfuged, there's no point in hiding 
it in a TARGET_PPC64 block, and thus you can drop the ifdefs.


r~