RE: [PATCH v2] Hexagon: move GETPC() calls to top level helpers

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Sent: Wednesday, July 5, 2023 12:35 PM
> To: qemu-devel@nongnu.org
> Cc: quic_mathbern@quicinc.com; bcain@quicinc.com;
> ltaylorsimpson@gmail.com; quic_mliebel@quicinc.com;
> richard.henderson@linaro.org
> Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> 
> As docs/devel/loads-stores.rst states:
> 
>   ``GETPC()`` should be used with great care: calling
>   it in other functions that are *not* the top level
>   ``HELPER(foo)`` will cause unexpected behavior. Instead, the
>   value of ``GETPC()`` should be read from the helper and passed
>   if needed to the functions that the helper calls.
> 
> Let's fix the GETPC() usage in Hexagon, making sure it's always called
from
> top level helpers and passed down to the places where it's needed. There
> are two snippets where that is not currently the case:
> 
> - probe_store(), which is only called from two helpers, so it's easy to
>   move GETPC() up.
> 
> - mem_load*() functions, which are also called directly from helpers,
>   but through the MEM_LOAD*() set of macros. Note that this are only
>   used when compiling with --disable-hexagon-idef-parser.
> 
>   In this case, we also take this opportunity to simplify the code,
>   unifying the mem_load*() functions.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
> v1:
> d40fabcf9d6e92e4cd8d6a144e9b2a9acf4580dc.1688420966.git.quic_mathber
> n@quicinc.com
> 
> Changes in v2:
> - Fixed wrong cpu_ld* unification from previous version.
> - Passed retaddr down to check_noshuf() and further, as Taylor
>   suggested.
> - Reorganized macros for simplification.
> 
>  target/hexagon/macros.h    | 19 ++++++------
>  target/hexagon/op_helper.h | 11 ++-----  target/hexagon/op_helper.c | 62
> +++++++++++---------------------------
>  3 files changed, 29 insertions(+), 63 deletions(-)
> 
> diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
> 5451b061ee..e44a932434 100644
> --- a/target/hexagon/macros.h
> +++ b/target/hexagon/macros.h
> @@ -173,15 +173,6 @@
>  #define fLOAD(NUM, SIZE, SIGN, EA, DST) \
> -    DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
> +    DST =  (size##SIZE##SIGN##_t)({ \
> +        check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
> +        MEM_LOAD##SIZE(env, EA, GETPC()); \
> +    })
>  #endif

This should be formatted as
#define fLOAD(...) \
    do { \
        check_noshuf(...); \
        DST = ...; \
    } while (0)

> a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index
> 8f3764d15e..7744e819ef 100644
> --- a/target/hexagon/op_helper.h
> +++ b/target/hexagon/op_helper.h
> +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
> +                  uint32_t slot, target_ulong vaddr, int size,
> +uintptr_t ra);

Are you sure this needs to be non-static?


Othersiwe
Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>