Re: [PATCH 23/67] target/arm: Convert FMOV, FABS, FNEG (scalar) to decodetree

[Richard Henderson]

On 12/5/24 15:12, Peter Maydell wrote:
> > @@ -8295,15 +8356,6 @@ static void handle_fp_1src_half(DisasContext *s, int 
> > opcode, int rd, int rn)
> >       TCGv_i32 tcg_res = tcg_temp_new_i32();
> >
> >       switch (opcode) {
> > -    case 0x0: /* FMOV */
> > -        tcg_gen_mov_i32(tcg_res, tcg_op);
> > -        break;
> > -    case 0x1: /* FABS */
> > -        gen_vfp_absh(tcg_res, tcg_op);
> > -        break;
> > -    case 0x2: /* FNEG */
> > -        gen_vfp_negh(tcg_res, tcg_op);
> > -        break;
> >       case 0x3: /* FSQRT */
> >           fpst = fpstatus_ptr(FPST_FPCR_F16);
> >           gen_helper_sqrt_f16(tcg_res, tcg_op, fpst);
> > @@ -8331,6 +8383,9 @@ static void handle_fp_1src_half(DisasContext *s, int 
> > opcode, int rd, int rn)
> >           gen_helper_advsimd_rinth(tcg_res, tcg_op, fpst);
> >           break;
> >       default:
> > +    case 0x0: /* FMOV */
> > +    case 0x1: /* FABS */
> > +    case 0x2: /* FNEG */
> >           g_assert_not_reached();
> >       }
>
> In these changes to the "handle this op" functions we make the
> function assert if it's passed an op we've converted. But shouldn't
> there also be a change which makes the calling function disas_fp_1src()
> call unallocated_encoding() for the ops ?

Yes.  I missed that because the line is

    case 0x0 ... 0x3:

without the usual set of comments.

> > @@ -10881,13 +10921,11 @@ static void disas_simd_two_reg_misc_fp16(DisasContext 
> > *s, uint32_t insn)
> >           case 0x7b: /* FCVTZU */
> >               gen_helper_advsimd_f16touinth(tcg_res, tcg_op, tcg_fpstatus);
> >               break;
> > -        case 0x6f: /* FNEG */
> > -            tcg_gen_xori_i32(tcg_res, tcg_op, 0x8000);
> > -            break;
> >           case 0x7d: /* FRSQRTE */
> >               gen_helper_rsqrte_f16(tcg_res, tcg_op, tcg_fpstatus);
> >               break;
> >           default:
> > +        case 0x6f: /* FNEG */
> >               g_assert_not_reached();
> >           }
>
> What's this change about? This bit of decode is not in the area that
> corresponds to the new patterns you add to a64.decode.
>
> Is this a bug in our existing decode where we decode something that
> should be undef? I think that 0x6f here corresponds to the decode
> table in section C4.1.96.6 ("Advanced SIMD scalar two-register
> miscellaneous FP16"), where it is U:a:opcode == 1 1 01111. But
> in that table that encoding is marked unallocated. A similar
> thing is true for case 0x2f FABS and case 07f FSQRT. All three
> of these should have set only_in_vector to true and not had the
> code handling in the is_scalar branch of the function.
>
> We should fix that bug in a separate patch before we do the
> decodetree conversion, I think.

You're right.  I had thought there was something weird here, in that FNEG was present but 
not FABS.  The only scalar FNEG is in fp data-processing 1-src.


r~