[1/9] target/arm: Implement vector shifted SCVF/UCVF for fp16

While we have some of the scalar paths for *CVF for fp16,
we failed to decode the fp16 version of these instructions.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-a64.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

Richard Henderson <richard.henderson@linaro.org> writes:

> While we have some of the scalar paths for *CVF for fp16,
> we failed to decode the fp16 version of these instructions.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate-a64.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index b47319d437..c92e052686 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -7077,13 +7077,26 @@ static void handle_simd_shift_intfp_conv(DisasContext *s, bool is_scalar,
>                                           int immh, int immb, int opcode,
>                                           int rn, int rd)
>  {
> -    bool is_double = extract32(immh, 3, 1);
> -    int size = is_double ? MO_64 : MO_32;
> -    int elements;
> +    int size, elements, fracbits;
>      int immhb = immh << 3 | immb;
> -    int fracbits = (is_double ? 128 : 64) - immhb;
>
> -    if (!extract32(immh, 2, 2)) {
> +    if (immh & 8) {
> +        size = MO_64;
> +        if (!is_scalar && !is_q) {
> +            unallocated_encoding(s);
> +            return;
> +        }
> +    } else if (immh & 4) {
> +        size = MO_32;
> +    } else if (immh & 2) {
> +        size = MO_16;
> +        if (!arm_dc_feature(s, ARM_FEATURE_V8_FP16)) {
> +            unallocated_encoding(s);
> +            return;
> +        }
> +    } else {
> +        /* immh == 0 would be a failure of the decode logic */
> +        g_assert(immh == 1);
>          unallocated_encoding(s);
>          return;
>      }
> @@ -7091,20 +7104,14 @@ static void handle_simd_shift_intfp_conv(DisasContext *s, bool is_scalar,
>      if (is_scalar) {
>          elements = 1;
>      } else {
> -        elements = is_double ? 2 : is_q ? 4 : 2;
> -        if (is_double && !is_q) {
> -            unallocated_encoding(s);
> -            return;
> -        }
> +        elements = 8 << is_q >> size;

That is a brain exercise for operator precedence. Would:

     elements = (is_q ? 16 : 8) >> size;

be clearer? Personally I'd have probably done it long hand in each size
leg above, e.g:

    size = MO_16;
    elements = is_scalar ? 1 : (is_q ? 8 : 4);

>      }
> +    fracbits = (16 << size) - immhb;

The ship has already sailed on this but I'm wishing we had a
mosize_to_bits() helper function to be explicit about this
transformation.

>
>      if (!fp_access_check(s)) {
>          return;
>      }
>
> -    /* immh == 0 would be a failure of the decode logic */
> -    g_assert(immh);
> -
>      handle_simd_intfp_conv(s, rd, rn, elements, !is_u, fracbits, size);
>  }


--
Alex Bennée

On 04/27/2018 11:04 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> While we have some of the scalar paths for *CVF for fp16,
>> we failed to decode the fp16 version of these instructions.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/arm/translate-a64.c | 33 ++++++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 13 deletions(-)
>>
>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> index b47319d437..c92e052686 100644
>> --- a/target/arm/translate-a64.c
>> +++ b/target/arm/translate-a64.c
>> @@ -7077,13 +7077,26 @@ static void handle_simd_shift_intfp_conv(DisasContext *s, bool is_scalar,
>>                                           int immh, int immb, int opcode,
>>                                           int rn, int rd)
>>  {
>> -    bool is_double = extract32(immh, 3, 1);
>> -    int size = is_double ? MO_64 : MO_32;
>> -    int elements;
>> +    int size, elements, fracbits;
>>      int immhb = immh << 3 | immb;
>> -    int fracbits = (is_double ? 128 : 64) - immhb;
>>
>> -    if (!extract32(immh, 2, 2)) {
>> +    if (immh & 8) {
>> +        size = MO_64;
>> +        if (!is_scalar && !is_q) {
>> +            unallocated_encoding(s);
>> +            return;
>> +        }
>> +    } else if (immh & 4) {
>> +        size = MO_32;
>> +    } else if (immh & 2) {
>> +        size = MO_16;
>> +        if (!arm_dc_feature(s, ARM_FEATURE_V8_FP16)) {
>> +            unallocated_encoding(s);
>> +            return;
>> +        }
>> +    } else {
>> +        /* immh == 0 would be a failure of the decode logic */
>> +        g_assert(immh == 1);
>>          unallocated_encoding(s);
>>          return;
>>      }
>> @@ -7091,20 +7104,14 @@ static void handle_simd_shift_intfp_conv(DisasContext *s, bool is_scalar,
>>      if (is_scalar) {
>>          elements = 1;
>>      } else {
>> -        elements = is_double ? 2 : is_q ? 4 : 2;
>> -        if (is_double && !is_q) {
>> -            unallocated_encoding(s);
>> -            return;
>> -        }
>> +        elements = 8 << is_q >> size;
> 
> That is a brain exercise for operator precedence. Would:
> 
>      elements = (is_q ? 16 : 8) >> size;
> 
> be clearer?

I don't think so myself.  I thought the double conditional harder to follow.

>> +    fracbits = (16 << size) - immhb;
> 
> The ship has already sailed on this but I'm wishing we had a
> mosize_to_bits() helper function to be explicit about this
> transformation.

Yeah, that might have been a good thing...


r~

On 29 April 2018 at 15:44, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 04/27/2018 11:04 AM, Alex Bennée wrote:
>>
>> Richard Henderson <richard.henderson@linaro.org> writes:
>>> +        elements = 8 << is_q >> size;
>>
>> That is a brain exercise for operator precedence. Would:
>>
>>      elements = (is_q ? 16 : 8) >> size;
>>
>> be clearer?
>
> I don't think so myself.

Can we at least put in the brackets if we're going to use
the << >> expression?

thanks
-- PMM