| Message ID | 20180425012300.14698-2-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Fixups for ARM_FEATURE_V8_FP16 | expand |
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
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; } + fracbits = (16 << size) - immhb; 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); }
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(-) -- 2.14.3