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

[Paolo Savini]

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~