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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 11/4/24 9:48 AM, Richard Henderson wrote:
> On 10/30/24 15:25, Paolo Savini wrote:
>> Thanks for the review Richard.
>>
>> On 10/30/24 11:40, Richard Henderson wrote:
>>> On 10/29/24 19:43, Paolo Savini wrote:
>>>> This patch optimizes the emulation of unit-stride load/store RVV instructions
>>>> when the data being loaded/stored per iteration amounts to 16 bytes or more.
>>>> The optimization consists of calling __builtin_memcpy on chunks of data of 16
>>>> bytes between the memory address of the simulated vector register and the
>>>> destination memory address and vice versa.
>>>> This is done only if we have direct access to the RAM of the host machine,
>>>> if the host is little endiand and if it supports atomic 128 bit memory
>>>> operations.
>>>>
>>>> Signed-off-by: Paolo Savini <paolo.savini@embecosm.com>
>>>> ---
>>>>   target/riscv/vector_helper.c    | 17 ++++++++++++++++-
>>>>   target/riscv/vector_internals.h | 12 ++++++++++++
>>>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
>>>> index 75c24653f0..e1c100e907 100644
>>>> --- a/target/riscv/vector_helper.c
>>>> +++ b/target/riscv/vector_helper.c
>>>> @@ -488,7 +488,22 @@ vext_group_ldst_host(CPURISCVState *env, void *vd, uint32_t byte_end,
>>>>       }
>>>>         fn = fns[is_load][group_size];
>>>> -    fn(vd, byte_offset, host + byte_offset);
>>>> +
>>>> +    /* __builtin_memcpy uses host 16 bytes vector loads and stores if supported.
>>>> +     * We need to make sure that these instructions have guarantees of atomicity.
>>>> +     * E.g. x86 processors provide strong guarantees of atomicity for 16-byte
>>>> +     * memory operations if the memory operands are 16-byte aligned */
>>>> +    if (!HOST_BIG_ENDIAN && (byte_offset + 16 < byte_end) &&
>>>> +            ((byte_offset % 16) == 0) && HOST_128_ATOMIC_MEM_OP) {
>>>> +      group_size = MO_128;
>>>> +      if (is_load) {
>>>> +        __builtin_memcpy((uint8_t *)(vd + byte_offset), (uint8_t *)(host + byte_offset), 16);
>>>> +      } else {
>>>> +        __builtin_memcpy((uint8_t *)(host + byte_offset), (uint8_t *)(vd + byte_offset), 16);
>>>> +      }
>>>
>>> I said this last time and I'll say it again:
>>>
>>>     __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.

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(...).

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

I'm curious to see if this form of front end optimization, use TCG ops
instead of a for() loop with ldst_elem() like we're doing today,  would yield more
performance with less complication with backend specifics, atomicity and whatnot.
In fact I have a feeling this is not the first time we talk about using ideas from
SVE too.


Thanks,

Daniel



  
> 
> r~