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

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

On 7/5/23 23:17, ltaylorsimpson@gmail.com wrote:
> 
> 
>> -----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)

Alternately,

     DST = (size##SIZE##SIGN##_t)mem_load##SIZE(env, pkt_has_store_s1, slot, EA, GETPC())

and retain the mem_loadN functions with an extra argument.

>> +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?

I think only if the mem_loadN functions are retained may it be static.
But perhaps the macro magic is actually limited to the same compilation unit?


r~