Re: [RFC v2 2/5] cputlb: Replace switches in load/store_helper with callback

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

On 9/19/19 7:53 AM, David Hildenbrand wrote:
> On 18.09.19 20:02, Richard Henderson wrote:
>> Add a function parameter to perform the actual load/store to ram.
>> With optimization, this results in identical code.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  accel/tcg/cputlb.c | 159 +++++++++++++++++++++++----------------------
>>  1 file changed, 83 insertions(+), 76 deletions(-)
> 
> I would have guessed the compiler propagates the constant and eliminates
> the switch completely for the variants. After all, we now have more LOC ...
> 
> I would have moved the actual read/write to a separate function
> containing the switch statement instead.

I tried that.

The compiler does fold away the constant and eliminate the switch for the code
as-is.  But it somehow fails to do so for op ^ MO_BSWAP, where op is constant.
 I don't know why.

These little inline functions were the third try and the only one for which all
of the indirection folded away.  Not so much here, but in the next patch which
passes in the reverse-endian function as well.


>> +/* Wrap the unaligned load helpers to that they have a common signature.  */
>> +static inline uint64_t wrap_ldub(const void *haddr)
>> +{
>> +    return ldub_p(haddr);
> 
> I wonder if you should just add proper type cast to all of the <
> uint64_t accessors (e.g., here (uint8_t) ). Shouldn't hurt and makes
> people wonder less how the conversion from the int these accessors
> return to uint64_t will turn out.
> 
> But yeah, you're simply moving code here.

Perhaps.  I'm not really a fan of redundant casts.


r~