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

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

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.


r~