Re: [RFC v4 2/2] target/riscv: rvv: improve performance of RISC-V vector loads and stores on large amounts of data.

[Paolo Savini <paolo.savini@embecosm.com>]

Hi Richard, Daniel,

This might be a silly question, but why do we need to ensure atomicity 
when emulating these guest instructions? I might be wrong but I didn't 
see an explicit requirement for the vector instructions to be atomic in 
the documentation of the RISC-V V extension.

Anyway the patches from Max have landed and since one of them already 
uses memcpy() where this patch does and achieves a similar performance 
improvement we should probably drop this particular patch. I'm wondering 
whether we should be concerned about atomicity there too?

https://github.com/qemu/qemu/blob/134b443512825bed401b6e141447b8cdc22d2efe/target/riscv/vector_helper.c#L224

Thanks

Paolo

On 11/8/24 09:11, Richard Henderson wrote:
> On 11/7/24 12:58, Daniel Henrique Barboza wrote:
>> On 11/4/24 9:48 AM, Richard Henderson wrote:
>>> On 10/30/24 15:25, Paolo Savini wrote:
>>>> On 10/30/24 11:40, Richard Henderson wrote:
>>>>>     __builtin_memcpy DOES NOT equal VMOVDQA
>>>> I am aware of this. I took __builtin_memcpy as a generic enough way 
>>>> to emulate loads and stores that should allow several hosts to 
>>>> generate the widest load/store instructions they can and on x86 I 
>>>> see this generates instructions vmovdpu/movdqu that are not always 
>>>> guaranteed to be atomic. x86 though guarantees them to be atomic if 
>>>> the memory address is aligned to 16 bytes.
>>>
>>> No, AMD guarantees MOVDQU is atomic if aligned, Intel does not.
>>> See the comment in util/cpuinfo-i386.c, and the two 
>>> CPUINFO_ATOMIC_VMOVDQ[AU] bits.
>>>
>>> See also host/include/*/host/atomic128-ldst.h, HAVE_ATOMIC128_RO, 
>>> and atomic16_read_ro.
>>> Not that I think you should use that here; it's complicated, and I 
>>> think you're better off relying on the code in accel/tcg/ when more 
>>> than byte atomicity is required.
>>>
>>
>> Not sure if that's what you meant but I didn't find any clear example of
>> multi-byte atomicity using qatomic_read() and friends that would be 
>> closer
>> to what memcpy() is doing here. I found one example in 
>> bdrv_graph_co_rdlock()
>> that seems to use a mem barrier via smp_mb() and qatomic_read() inside a
>> loop, but I don't understand that code enough to say.
>
> Memory barriers provide ordering between loads and stores, but they 
> cannot be used to address atomicity of individual loads and stores.
>
>
>> I'm also wondering if a common pthread_lock() wrapping up these 
>> memcpy() calls
>> would suffice in this case. Even if we can't guarantee that 
>> __builtin_memcpy()
>> will use arch specific vector insns in the host it would already be a 
>> faster
>> path than falling back to fn(...).
>
> Locks would certainly not be faster than calling the accel/tcg function.
>
>
>> In a quick detour, I'm not sure if we really considered how ARM SVE 
>> implements these
>> helpers. E.g gen_sve_str():
>>
>> https://gitlab.com/qemu-project/qemu/-/blob/master/target/arm/tcg/translate-sve.c#L4182 
>>
>
> Note that ARM SVE defines these instructions to have byte atomicity.
>
>
> r~