Re: [PATCH v5 23/55] translator: add translator_ld{ub,sw,uw,l,q}

[Alex Bennée]

Peter Maydell <address@hidden> writes:

> On Mon, 14 Oct 2019 at 12:38, Alex Bennée <address@hidden> wrote:
>>
>> From: "Emilio G. Cota" <address@hidden>
>>
>> We don't bother with replicating the fast path (tlb_hit) of the old
>> cpu_ldst helpers as it has no measurable effect on performance. This
>> probably indicates we should consider flattening the whole set of
>> helpers but that is out of scope for this change.
>>
>> Suggested-by: Richard Henderson <address@hidden>
>> Signed-off-by: Emilio G. Cota <address@hidden>
>> [AJB: directly plumb into softmmu/user helpers]
>> Signed-off-by: Alex Bennée <address@hidden>
>>
>> diff --git a/tcg/tcg.h b/tcg/tcg.h
>> index a38659ea5b..302533b463 100644
>> --- a/tcg/tcg.h
>> +++ b/tcg/tcg.h
>> @@ -1317,6 +1317,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, 
>> target_ulong addr,
>>  # define helper_ret_stl_mmu   helper_be_stl_mmu
>>  # define helper_ret_stq_mmu   helper_be_stq_mmu
>>  # define helper_ret_ldw_cmmu  helper_be_ldw_cmmu
>> +# define helper_ret_lduw_cmmu helper_be_ldw_cmmu
>>  # define helper_ret_ldl_cmmu  helper_be_ldl_cmmu
>>  # define helper_ret_ldq_cmmu  helper_be_ldq_cmmu
>>  #else
>> @@ -1330,6 +1331,7 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, 
>> target_ulong addr,
>>  # define helper_ret_stl_mmu   helper_le_stl_mmu
>>  # define helper_ret_stq_mmu   helper_le_stq_mmu
>>  # define helper_ret_ldw_cmmu  helper_le_ldw_cmmu
>> +# define helper_ret_lduw_cmmu helper_le_ldw_cmmu
>>  # define helper_ret_ldl_cmmu  helper_le_ldl_cmmu
>>  # define helper_ret_ldq_cmmu  helper_le_ldq_cmmu
>>  #endif
>
> This looks odd. Why is it ok to define a 'lduw' helper
> as the 'ldw' cmmu helper ? One ought to be sign
> extending and the other not...

This is the alternative:

3 files changed, 9 insertions(+), 17 deletions(-)
include/exec/translator.h | 19 +++++++++----------
include/qemu/bswap.h      |  5 -----
tcg/tcg.h                 |  2 --

modified   include/exec/translator.h
@@ -158,26 +158,26 @@ void translator_loop_temp_check(DisasContextBase *db);

 #ifdef CONFIG_USER_ONLY

-#define DO_LOAD(type, name, shift)               \
+#define DO_LOAD(type, name, uname, shift)        \
     set_helper_retaddr(1);                       \
-    ret = name ## _p(g2h(pc));                   \
+    ret = uname ## _p(g2h(pc));                  \
     clear_helper_retaddr();

 #else

-#define DO_LOAD(type, name, shift)                   \
+#define DO_LOAD(type, name, uname, shift)            \
     int mmu_idx = cpu_mmu_index(env, true);          \
     TCGMemOpIdx oi = make_memop_idx(shift, mmu_idx); \
     ret = helper_ret_ ## name ## _cmmu(env, pc, oi, 0);

 #endif

-#define GEN_TRANSLATOR_LD(fullname, name, type, shift, swap_fn)         \
+#define GEN_TRANSLATOR_LD(fullname, name, uname, type, shift, swap_fn)  \
     static inline type                                                  \
     fullname ## _swap(CPUArchState *env, abi_ptr pc, bool do_swap)      \
     {                                                                   \
         type ret;                                                       \
-        DO_LOAD(type, name, shift)                                      \
+        DO_LOAD(type, name, uname, shift)                               \
                                                                         \
         if (do_swap) {                                                  \
             ret = swap_fn(ret);                                         \
@@ -191,11 +191,10 @@ void translator_loop_temp_check(DisasContextBase *db);
         return fullname ## _swap(env, pc, false);                       \
     }

-GEN_TRANSLATOR_LD(translator_ldub, ldb, uint8_t, 0, /* no swap needed */)
-GEN_TRANSLATOR_LD(translator_ldsw, lduw, int16_t, 1, bswap16)
-GEN_TRANSLATOR_LD(translator_lduw, lduw, uint16_t, 1, bswap16)
-GEN_TRANSLATOR_LD(translator_ldl, ldl, uint32_t, 2, bswap32)
-GEN_TRANSLATOR_LD(translator_ldq, ldq, uint64_t, 3, bswap64)
+GEN_TRANSLATOR_LD(translator_ldub, ldb, ldub, uint8_t, 0, /* no swap needed */)
+GEN_TRANSLATOR_LD(translator_ldw, ldw, lduw, uint16_t, 1, bswap16)
+GEN_TRANSLATOR_LD(translator_ldl, ldl, ldl, uint32_t, 2, bswap32)
+GEN_TRANSLATOR_LD(translator_ldq, ldq, ldl, uint64_t, 3, bswap64)
 #undef GEN_TRANSLATOR_LD

 #endif  /* EXEC__TRANSLATOR_H */
modified   include/qemu/bswap.h
@@ -306,11 +306,6 @@ static inline int ldub_p(const void *ptr)
     return *(uint8_t *)ptr;
 }

-static inline int ldb_p(const void *ptr)
-{
-    return ldub_p(ptr);
-}
-
 static inline int ldsb_p(const void *ptr)
 {
     return *(int8_t *)ptr;
modified   tcg/tcg.h
@@ -1317,7 +1317,6 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, 
target_ulong addr,
 # define helper_ret_stl_mmu   helper_be_stl_mmu
 # define helper_ret_stq_mmu   helper_be_stq_mmu
 # define helper_ret_ldw_cmmu  helper_be_ldw_cmmu
-# define helper_ret_lduw_cmmu helper_be_ldw_cmmu
 # define helper_ret_ldl_cmmu  helper_be_ldl_cmmu
 # define helper_ret_ldq_cmmu  helper_be_ldq_cmmu
 #else
@@ -1331,7 +1330,6 @@ uint64_t helper_be_ldq_cmmu(CPUArchState *env, 
target_ulong addr,
 # define helper_ret_stl_mmu   helper_le_stl_mmu
 # define helper_ret_stq_mmu   helper_le_stq_mmu
 # define helper_ret_ldw_cmmu  helper_le_ldw_cmmu
-# define helper_ret_lduw_cmmu helper_le_ldw_cmmu
 # define helper_ret_ldl_cmmu  helper_le_ldl_cmmu
 # define helper_ret_ldq_cmmu  helper_le_ldq_cmmu
 #endif

--
Alex Bennée