Re: [PATCH v5 13/14] Hexagon (target/hexagon) Reduce manipulation of slot_cancelled

From: Anton Johansson via <qemu-devel@nongnu.org>
To: Taylor Simpson <tsimpson@quicinc.com>, qemu-devel@nongnu.org
Cc: richard.henderson@linaro.org, philmd@linaro.org, ale@rev.ng,
	bcain@quicinc.com, quic_mathbern@quicinc.com
Subject: Re: [PATCH v5 13/14] Hexagon (target/hexagon) Reduce manipulation of slot_cancelled
Date: Fri, 24 Feb 2023 15:24:24 +0100	[thread overview]
Message-ID: <2eb2c248-8a14-345c-1a54-7c28a1458123@rev.ng> (raw)
In-Reply-To: <20230131225647.25274-14-tsimpson@quicinc.com>


On 1/31/23 23:56, Taylor Simpson wrote:
>   /* Called during packet commit when there are two scalar stores */
> -void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int mmu_idx)
> +void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int args)
>   {
> -    probe_store(env, 0, mmu_idx);
> +    int mmu_idx = args & 0x3;
> +    bool is_predicated = (args >> 2) & 1;
> +    probe_store(env, 0, mmu_idx, is_predicated);
>   }
Can we use bitmasks for the fields of args?
>   void HELPER(probe_hvx_stores)(CPUHexagonState *env, int mmu_idx)
> @@ -489,12 +492,14 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask,
>       bool has_st0        = (mask >> 0) & 1;
>       bool has_st1        = (mask >> 1) & 1;
>       bool has_hvx_stores = (mask >> 2) & 1;
> +    bool s0_is_pred     = (mask >> 3) & 1;
> +    bool s1_is_pred     = (mask >> 4) & 1;
also used here
>       if (has_st0) {
> -        probe_store(env, 0, mmu_idx);
> +        probe_store(env, 0, mmu_idx, s0_is_pred);
>       }
>       if (has_st1) {
> -        probe_store(env, 1, mmu_idx);
> +        probe_store(env, 1, mmu_idx, s1_is_pred);
>       }
>       if (has_hvx_stores) {
>           HELPER(probe_hvx_stores)(env, mmu_idx);
> @@ -1444,12 +1449,6 @@ void HELPER(vwhist128qm)(CPUHexagonState *env, int32_t uiV)
>       }
>   }
>   
> -void cancel_slot(CPUHexagonState *env, uint32_t slot)
> -{
> -    HEX_DEBUG_LOG("Slot %d cancelled\n", slot);
> -    env->slot_cancelled |= (1 << slot);
> -}
> -
>   /* These macros can be referenced in the generated helper functions */
>   #define warn(...) /* Nothing */
>   #define fatal(...) g_assert_not_reached();
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
> index 53fd935db7..6ee8784910 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -248,7 +248,16 @@ static bool check_for_attrib(Packet *pkt, int attrib)
>   
>   static bool need_slot_cancelled(Packet *pkt)
>   {
> -    return check_for_attrib(pkt, A_CONDEXEC);
> +    /* We only need slot_cancelled for conditional store and HVX instructions */
> +    for (int i = 0; i < pkt->num_insns; i++) {
> +        uint16_t opcode = pkt->insn[i].opcode;
> +        if (GET_ATTRIB(opcode, A_CONDEXEC) &&
> +            (GET_ATTRIB(opcode, A_STORE) ||
> +             GET_ATTRIB(opcode, A_CVI))) {
> +            return true;
> +        }
> +    }
> +    return false;
>   }
>   
>   static bool need_pred_written(Packet *pkt)
> @@ -860,6 +869,12 @@ static void gen_commit_packet(DisasContext *ctx)
>               if (has_hvx_store) {
>                   mask |= (1 << 2);
>               }
> +            if (has_store_s0 && slot_is_predicated(pkt, 0)) {
> +                mask |= (1 << 3);
> +            }
> +            if (has_store_s1 && slot_is_predicated(pkt, 1)) {
> +                mask |= (1 << 4);
> +            }
>               mask_tcgv = tcg_constant_tl(mask);
>               gen_helper_probe_pkt_scalar_hvx_stores(cpu_env, mask_tcgv, mem_idx);
>           }
and here
> @@ -868,8 +883,12 @@ static void gen_commit_packet(DisasContext *ctx)
>            * process_store_log will execute the slot 1 store first,
>            * so we only have to probe the store in slot 0
>            */
> -        TCGv mem_idx = tcg_constant_tl(ctx->mem_idx);
> -        gen_helper_probe_pkt_scalar_store_s0(cpu_env, mem_idx);
> +        int args = ctx->mem_idx;
> +        if (slot_is_predicated(pkt, 0)) {
> +            args |= (1 << 2);
> +        }
> +        TCGv args_tcgv = tcg_constant_tl(args);
> +        gen_helper_probe_pkt_scalar_store_s0(cpu_env, args_tcgv);
>       }
and here

Otherwise,

Reviewed-by: Anton Johansson <anjo@rev.ng>