Re: [PATCH 3/3] Hexagon (target/hexagon) Change decision to set pkt_has_store_s[01]

[Richard Henderson <richard.henderson@linaro.org>]

On 9/20/22 01:07, Taylor Simpson wrote:
> We have found cases where pkt_has_store_s[01] is set incorrectly.
> This leads to generating an unnecessary store that is left over
> from a previous packet.
> 
> Add an attribute to determine if an instruction is a scalar store
> The attribute is attached to the fSTORE macro (hex_common.py)
> Simplify the logic in decode.c that sets pkt_has_store_s[01]
> 
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
>   target/hexagon/attribs_def.h.inc |  1 +
>   target/hexagon/decode.c          | 17 ++++++++++++-----
>   target/hexagon/translate.c       | 10 ++++++----
>   target/hexagon/hex_common.py     |  3 ++-
>   4 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/target/hexagon/attribs_def.h.inc b/target/hexagon/attribs_def.h.inc
> index 222ad95fb0..5d2a102c18 100644
> --- a/target/hexagon/attribs_def.h.inc
> +++ b/target/hexagon/attribs_def.h.inc
> @@ -44,6 +44,7 @@ DEF_ATTRIB(MEMSIZE_1B, "Memory width is 1 byte", "", "")
>   DEF_ATTRIB(MEMSIZE_2B, "Memory width is 2 bytes", "", "")
>   DEF_ATTRIB(MEMSIZE_4B, "Memory width is 4 bytes", "", "")
>   DEF_ATTRIB(MEMSIZE_8B, "Memory width is 8 bytes", "", "")
> +DEF_ATTRIB(SCALAR_STORE, "Store is scalar", "", "")
>   DEF_ATTRIB(REGWRSIZE_1B, "Memory width is 1 byte", "", "")
>   DEF_ATTRIB(REGWRSIZE_2B, "Memory width is 2 bytes", "", "")
>   DEF_ATTRIB(REGWRSIZE_4B, "Memory width is 4 bytes", "", "")
> diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
> index 6f0f27b4ba..2ba94a77de 100644
> --- a/target/hexagon/decode.c
> +++ b/target/hexagon/decode.c
> @@ -1,5 +1,5 @@
>   /*
> - *  Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved.
> + *  Copyright(c) 2019-2022 Qualcomm Innovation Center, Inc. All Rights Reserved.
>    *
>    *  This program is free software; you can redistribute it and/or modify
>    *  it under the terms of the GNU General Public License as published by
> @@ -402,10 +402,17 @@ static void decode_set_insn_attr_fields(Packet *pkt)
>           }
>   
>           if (GET_ATTRIB(opcode, A_STORE)) {
> -            if (pkt->insn[i].slot == 0) {
> -                pkt->pkt_has_store_s0 = true;
> -            } else {
> -                pkt->pkt_has_store_s1 = true;
> +            if (GET_ATTRIB(opcode, A_SCALAR_STORE) &&
> +                !GET_ATTRIB(opcode, A_MEMSIZE_0B)) {
> +                g_assert(GET_ATTRIB(opcode, A_MEMSIZE_1B) ||
> +                         GET_ATTRIB(opcode, A_MEMSIZE_2B) ||
> +                         GET_ATTRIB(opcode, A_MEMSIZE_4B) ||
> +                         GET_ATTRIB(opcode, A_MEMSIZE_8B));

Would this assert be redundant with the one I suggested vs patch 2?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> +                if (pkt->insn[i].slot == 0) {
> +                    pkt->pkt_has_store_s0 = true;
> +                } else {
> +                    pkt->pkt_has_store_s1 = true;
> +                }
>               }
>           }
>   
> diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
> index bc02870b9f..efe7d2264e 100644
> --- a/target/hexagon/translate.c
> +++ b/target/hexagon/translate.c
> @@ -1,5 +1,5 @@
>   /*
> - *  Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved.
> + *  Copyright(c) 2019-2022 Qualcomm Innovation Center, Inc. All Rights Reserved.
>    *
>    *  This program is free software; you can redistribute it and/or modify
>    *  it under the terms of the GNU General Public License as published by
> @@ -525,10 +525,12 @@ static void process_store_log(DisasContext *ctx, Packet *pkt)
>        *  slot 1 and then slot 0.  This will be important when
>        *  the memory accesses overlap.
>        */
> -    if (pkt->pkt_has_store_s1 && !pkt->pkt_has_dczeroa) {
> +    if (pkt->pkt_has_store_s1) {
> +        g_assert(!pkt->pkt_has_dczeroa);
>           process_store(ctx, pkt, 1);
>       }
> -    if (pkt->pkt_has_store_s0 && !pkt->pkt_has_dczeroa) {
> +    if (pkt->pkt_has_store_s0) {
> +        g_assert(!pkt->pkt_has_dczeroa);
>           process_store(ctx, pkt, 0);
>       }
>   }
> @@ -691,7 +693,7 @@ static void gen_commit_packet(CPUHexagonState *env, DisasContext *ctx,
>            * The dczeroa will be the store in slot 0, check that we don't have
>            * a store in slot 1 or an HVX store.
>            */
> -        g_assert(has_store_s0 && !has_store_s1 && !has_hvx_store);
> +        g_assert(!has_store_s1 && !has_hvx_store);
>           process_dczeroa(ctx, pkt);
>       } else if (has_hvx_store) {
>           TCGv mem_idx = tcg_constant_tl(ctx->mem_idx);
> diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
> index c81aca8d2a..d9ba7df786 100755
> --- a/target/hexagon/hex_common.py
> +++ b/target/hexagon/hex_common.py
> @@ -1,7 +1,7 @@
>   #!/usr/bin/env python3
>   
>   ##
> -##  Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved.
> +##  Copyright(c) 2019-2022 Qualcomm Innovation Center, Inc. All Rights Reserved.
>   ##
>   ##  This program is free software; you can redistribute it and/or modify
>   ##  it under the terms of the GNU General Public License as published by
> @@ -75,6 +75,7 @@ def calculate_attribs():
>       add_qemu_macro_attrib('fWRITE_P3', 'A_WRITES_PRED_REG')
>       add_qemu_macro_attrib('fSET_OVERFLOW', 'A_IMPLICIT_WRITES_USR')
>       add_qemu_macro_attrib('fSET_LPCFG', 'A_IMPLICIT_WRITES_USR')
> +    add_qemu_macro_attrib('fSTORE', 'A_SCALAR_STORE')
>   
>       # Recurse down macros, find attributes from sub-macros
>       macroValues = list(macros.values())