[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 14/19] target/arm: Move the vfp decodetree calls next to the
From: |
Peter Maydell |
Subject: |
Re: [PATCH 14/19] target/arm: Move the vfp decodetree calls next to the base isa |
Date: |
Thu, 20 Feb 2020 17:16:21 +0000 |
On Fri, 14 Feb 2020 at 18:16, Richard Henderson
<address@hidden> wrote:
>
> Have the calls adjacent as an intermediate step toward
> actually merging the decodes.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
> target/arm/translate.c | 80 +++++++++++++-----------------------------
> 1 file changed, 24 insertions(+), 56 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index b2641b4262..5cabe6b2e9 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -2646,31 +2646,6 @@ static void gen_neon_dup_high16(TCGv_i32 var)
> tcg_temp_free_i32(tmp);
> }
>
> -/*
> - * Disassemble a VFP instruction. Returns nonzero if an error occurred
> - * (ie. an undefined instruction).
> - */
> -static int disas_vfp_insn(DisasContext *s, uint32_t insn)
> -{
> - /*
> - * If the decodetree decoder handles this insn it will always
> - * emit code to either execute the insn or generate an appropriate
> - * exception; so we don't need to ever return non-zero to tell
> - * the calling code to emit an UNDEF exception.
> - */
> - if (extract32(insn, 28, 4) == 0xf) {
> - if (disas_vfp_uncond(s, insn)) {
> - return 0;
> - }
> - } else {
> - if (disas_vfp(s, insn)) {
> - return 0;
> - }
> - }
> - /* If the decodetree decoder didn't handle this insn, it must be UNDEF */
> - return 1;
> -}
Before this change, if this was a cp10/11 insn and
neither disas_vfp_uncond() nor disas_vfp() returned true,
we would UNDEF it.
> -
> static inline bool use_goto_tb(DisasContext *s, target_ulong dest)
> {
> #ifndef CONFIG_USER_ONLY
> @@ -10524,7 +10499,9 @@ static void disas_arm_insn(DisasContext *s, unsigned
> int insn)
> ARCH(5);
>
> /* Unconditional instructions. */
> - if (disas_a32_uncond(s, insn)) {
> + /* TODO: Perhaps merge these into one decodetree output file. */
> + if (disas_a32_uncond(s, insn) ||
> + disas_vfp_uncond(s, insn)) {
> return;
> }
> /* fall back to legacy decoder */
> @@ -10551,13 +10528,6 @@ static void disas_arm_insn(DisasContext *s, unsigned
> int insn)
> }
> return;
> }
> - if ((insn & 0x0f000e10) == 0x0e000a00) {
> - /* VFP. */
> - if (disas_vfp_insn(s, insn)) {
> - goto illegal_op;
> - }
> - return;
> - }
> if ((insn & 0x0e000f00) == 0x0c000100) {
> if (arm_dc_feature(s, ARM_FEATURE_IWMMXT)) {
> /* iWMMXt register transfer. */
> @@ -10588,7 +10558,9 @@ static void disas_arm_insn(DisasContext *s, unsigned
> int insn)
> arm_skip_unless(s, cond);
> }
>
> - if (disas_a32(s, insn)) {
> + /* TODO: Perhaps merge these into one decodetree output file. */
> + if (disas_a32(s, insn) ||
> + disas_vfp(s, insn)) {
> return;
> }
> /* fall back to legacy decoder */
> @@ -10597,12 +10569,7 @@ static void disas_arm_insn(DisasContext *s, unsigned
> int insn)
> case 0xc:
> case 0xd:
> case 0xe:
> - if (((insn >> 8) & 0xe) == 10) {
> - /* VFP. */
> - if (disas_vfp_insn(s, insn)) {
> - goto illegal_op;
> - }
> - } else if (disas_coproc_insn(s, insn)) {
> + if (((insn >> 8) & 0xe) != 10 && disas_coproc_insn(s, insn)) {
> /* Coprocessor. */
> goto illegal_op;
> }
But now if the VFP decodetree doesn't handle the insn,
we'll fall into this case here, I think, the
"(((insn >> 8) & 0xe) != 10" part of the condition will
be false, and we'll end up at a 'break' statement, which
I think means we'll end up doing a NOP rather than an UNDEF.
> @@ -10691,7 +10658,14 @@ static void disas_thumb2_insn(DisasContext *s,
> uint32_t insn)
> ARCH(6T2);
> }
>
> - if (disas_t32(s, insn)) {
> + /*
> + * TODO: Perhaps merge these into one decodetree output file.
> + * Note disas_vfp is written for a32 with cond field in the
> + * top nibble. The t32 encoding requires 0xe in the top nibble.
> + */
> + if (disas_t32(s, insn) ||
> + disas_vfp_uncond(s, insn) ||
> + ((insn >> 28) == 0xe && disas_vfp(s, insn))) {
> return;
> }
> /* fall back to legacy decoder */
> @@ -10708,17 +10682,15 @@ static void disas_thumb2_insn(DisasContext *s,
> uint32_t insn)
> goto illegal_op; /* op0 = 0b11 : unallocated */
> }
>
> - if (disas_vfp_insn(s, insn)) {
> - if (((insn >> 8) & 0xe) == 10 &&
> - dc_isar_feature(aa32_fpsp_v2, s)) {
> - /* FP, and the CPU supports it */
> - goto illegal_op;
> - } else {
> - /* All other insns: NOCP */
> - gen_exception_insn(s, s->pc_curr, EXCP_NOCP,
> - syn_uncategorized(),
> - default_exception_el(s));
> - }
> + if (((insn >> 8) & 0xe) == 10 &&
> + dc_isar_feature(aa32_fpsp_v2, s)) {
> + /* FP, and the CPU supports it */
> + goto illegal_op;
> + } else {
> + /* All other insns: NOCP */
> + gen_exception_insn(s, s->pc_curr, EXCP_NOCP,
> + syn_uncategorized(),
> + default_exception_el(s));
> }
> break;
> }
> @@ -10740,10 +10712,6 @@ static void disas_thumb2_insn(DisasContext *s,
> uint32_t insn)
> if (disas_neon_data_insn(s, insn)) {
> goto illegal_op;
> }
> - } else if (((insn >> 8) & 0xe) == 10) {
> - if (disas_vfp_insn(s, insn)) {
> - goto illegal_op;
> - }
> } else {
> if (insn & (1 << 28))
> goto illegal_op;
Here in the thumb decoder there's a similar issue I think:
now for insns which are in the VFP space but don't exist
(ie where the decodetree has returned false) we'll end up
now falling through into disas_coproc_insn() rather than
just UNDEFing them.
thanks
-- PMM
- [PATCH 10/19] target/arm: Add missing checks for fpsp_v2, (continued)
- [PATCH 10/19] target/arm: Add missing checks for fpsp_v2, Richard Henderson, 2020/02/14
- [PATCH 11/19] target/arm: Replace ARM_FEATURE_VFP4 with isar_feature_aa32_simdfmac, Richard Henderson, 2020/02/14
- [PATCH 13/19] target/arm: Move VLLDM and VLSTM to vfp.decode, Richard Henderson, 2020/02/14
- [PATCH 15/19] linux-user/arm: Replace ARM_FEATURE_VFP* tests for HWCAP, Richard Henderson, 2020/02/14
- [PATCH 14/19] target/arm: Move the vfp decodetree calls next to the base isa, Richard Henderson, 2020/02/14
- Re: [PATCH 14/19] target/arm: Move the vfp decodetree calls next to the base isa,
Peter Maydell <=
- [PATCH 16/19] target/arm: Remove ARM_FEATURE_VFP*, Richard Henderson, 2020/02/14
- [PATCH 18/19] target/arm: Split VFM decode, Richard Henderson, 2020/02/14
- [PATCH 17/19] target/arm: Add formats for some vfp 2 and 3-register insns, Richard Henderson, 2020/02/14
- [PATCH 19/19] target/arm: Split VMINMAXNM decode, Richard Henderson, 2020/02/14
- Re: [PATCH 00/19] target/arm: vfp feature and decodetree cleanup, no-reply, 2020/02/14
- Re: [PATCH 00/19] target/arm: vfp feature and decodetree cleanup, Peter Maydell, 2020/02/20