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

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/helper.h                        |  1 +
 target/ppc/insn32.decode                   |  4 +++
 target/ppc/int_helper.c                    | 39 ++++++++++++++++++++++
 target/ppc/translate/fixedpoint-impl.c.inc | 16 +++++++--
 4 files changed, 58 insertions(+), 2 deletions(-)

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~

On 13/05/2021 08:31, Richard Henderson wrote:
> 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.
> 

It sure worth more comments. Yes, the idea is to process in blocks, and 
we negate the mask to avoid deciding between ctz and cto inside the 
loop. We use rotate instead of shift so it don't change the number of 
zeros and ones, and then we don't need orig_mask.

You'll find my test cases for cfuged and vcfuged on 
https://github.com/PPC64/qemu/blob/ferst-tcg-cfuged/tests/tcg/ppc64le/ . 
I got the same results by running them with this implementation and with 
the Power10 Functional Simulator.

>> +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~
> 

If I remove it, the build for ppc will fail, because cpu_gpr is declared 
as TCGv, and the helper uses i64 to match {get,set}_cpu_vsr{l,h}. 
REQUIRE_64BIT makes the helper call unreachable for ppc, but it's a 
runtime check. At build time, the compiler will check the types anyway, 
and give us an error.

On 5/13/21 7:24 AM, Matheus K. Ferst wrote:
>>> +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~
>>
> 
> If I remove it, the build for ppc will fail, because cpu_gpr is declared as 
> TCGv, and the helper uses i64 to match {get,set}_cpu_vsr{l,h}. REQUIRE_64BIT 
> makes the helper call unreachable for ppc, but it's a runtime check. At build 
> time, the compiler will check the types anyway, and give us an error.

Hmm, yes.  Just change the gen_invalid above to qemu_build_not_reached().


r~