Re: [PATCH 1/1] riscv: add vectorized memset, memcpy and memmove

Hi Kito,

Sorry for the late reply.

On 2025/12/12 16:33, Kito Cheng wrote:
>> diff --git a/newlib/libc/machine/riscv/memcpy-asm.S b/newlib/libc/machine/riscv/memcpy-asm.S
>> index 2771285f9..9d1d2d4bd 100644
>> --- a/newlib/libc/machine/riscv/memcpy-asm.S
>> +++ b/newlib/libc/machine/riscv/memcpy-asm.S
>> @@ -9,11 +9,11 @@
>>      http://www.opensource.org/licenses.
>>   */
>>
>> -#if defined(PREFER_SIZE_OVER_SPEED) || defined(__OPTIMIZE_SIZE__)
>>   .text
>>   .global memcpy
>>   .type  memcpy, @function
>>   memcpy:
>> +#if defined(PREFER_SIZE_OVER_SPEED) || defined(__OPTIMIZE_SIZE__)
> 
> This seems not right change to me, memcpy-asm.S is NOT conditional
> compile in the Makefile, so that mean if we didn't defined
> PREFER_SIZE_OVER_SPEED, __OPTIMIZE_SIZE__ or __riscv_v, we will have a
> empty memcpy in memcpy-asm.o and then this will included in libc.a
> 
> Same issue for memmove-asm.S
> 

My apologies for not implementing and testing the changes thoroughly 
enough. I'll move the guard back to its original position in the next 
revision.

>>     mv a3, a0
>>     beqz a2, 2f
>>
>> @@ -29,4 +29,25 @@ memcpy:
>>     ret
>>
>>     .size        memcpy, .-memcpy
>> +#elif defined(__riscv_v)
> 
> Suggest use __riscv_vector rather than __riscv_v, so that we can also
> use that logic for zve* extensions.
> 
>> +  .option push
>> +  .option arch, +v
> 
> and arch, +zve32x here rather than +v
> 

Will replace macro and arch,+v to support both full V and Zve* extensions.

>> +  mv      t0, a0                    /* running dst */
>> +  mv      t1, a1                    /* running src */
>> +  beqz    a2, .Ldone_copy           /* n == 0 then return */
>> +
>> +.Lbulk_copy:
>> +  vsetvli t2, a2, e8, m8, ta, ma    /* t2 = vl (bytes) */
>> +  vle8.v  v0, (t1)
>> +  vse8.v  v0, (t0)
>> +  add     t0, t0, t2
>> +  add     t1, t1, t2
>> +  sub     a2, a2, t2
> 
> This sub can be drop
> 
>> +  bnez    a2, .Lbulk_copy
> 
> You can use either src(a1)+len(a2) or dst(a0)+len(a2) for loop condition:
> 
> something like:
> 
> void *
> memcpy(unsigned char *dst, const unsigned char *src,
>                         const size_t sz)
> {
>   const unsigned char *end = dst + sz;
>   while (dst != end)
>     *dst++ = *src++;
>    return dst;
> }
> 
> This optimization could be applied on other function as well
> 

Thanks for the suggestion. Will restructure the loop conditions as you 
suggested.

>> +  /* fallthrough */
>> +
>> +.Ldone_copy:
>> +  ret
>> +.size memcpy, .-memcpy
>> +.option pop
>>   #endif
>> diff --git a/newlib/libc/machine/riscv/memcpy.c b/newlib/libc/machine/riscv/memcpy.c
>> index a27e0ecb1..cd58c30a5 100644
>> --- a/newlib/libc/machine/riscv/memcpy.c
>> +++ b/newlib/libc/machine/riscv/memcpy.c
>> @@ -10,7 +10,7 @@
>>      http://www.opensource.org/licenses.
>>   */
>>
>> -#if defined(PREFER_SIZE_OVER_SPEED) || defined(__OPTIMIZE_SIZE__)
>> +#if defined(PREFER_SIZE_OVER_SPEED) || defined(__OPTIMIZE_SIZE__) || defined(__riscv_v)
>>   // memcpy defined in memcpy-asm.S
>>   #else
>>
>> diff --git a/newlib/libc/machine/riscv/memmove-asm.S b/newlib/libc/machine/riscv/memmove-asm.S
>> index 061472ca2..5cc2e5143 100644
>> --- a/newlib/libc/machine/riscv/memmove-asm.S
>> +++ b/newlib/libc/machine/riscv/memmove-asm.S
>> @@ -9,11 +9,11 @@
>>      http://www.opensource.org/licenses.
>>   */
>>
>> -#if defined(PREFER_SIZE_OVER_SPEED) || defined(__OPTIMIZE_SIZE__)
>>   .text
>>   .global memmove
>>   .type  memmove, @function
>>   memmove:
>> +#if defined(PREFER_SIZE_OVER_SPEED) || defined(__OPTIMIZE_SIZE__)
>>     beqz a2, .Ldone              /* in case there are 0 bytes to be copied, return immediately */
>>
>>     mv a4, a0                    /* copy the destination address over to a4, since memmove should return that address in a0 at the end */
>> @@ -37,4 +37,49 @@ memmove:
>>     ret
>>
>>     .size        memmove, .-memmove
>> +#elif defined(__riscv_v)
>> +  .option push
>> +  .option arch, +v
>> +  beqz    a2, .Ldone_move           /* n == 0 */
>> +  beq     a0, a1, .Ldone_move       /* dst == src */
>> +
>> +  /* overlap check */
>> +  bgeu    a1, a0, .Lforward_move    /* src >= dst then forward move*/
>> +
>> +  sub     t2, a0, a1                /* t2 = dst - src */
>> +  bgeu    t2, a2, .Lforward_move    /* no overlap then forward move */
>> +
>> +  /* backward move */
>> +  add     t0, a0, a2                /* running dst_end */
>> +  add     t1, a1, a2                /* running src_end */
>> +
>> +.Lbackward_loop:
>> +  vsetvli t3, a2, e8, m8, ta, ma    /* t3 = vl (bytes) */
>> +  sub     t0, t0, t3
>> +  sub     t1, t1, t3
>> +  vle8.v  v0, (t1)
>> +  vse8.v  v0, (t0)
>> +  sub     a2, a2, t3
>> +  bnez    a2, .Lbackward_loop
>> +  j       .Ldone_move
> 
> `ret` rather than `j       .Ldone_move` here, ret and j are both one
> instruction, so let just return to save one more jump :)

Absolutely.I'll change in the next revision.

Thank you very much for the detailed and thoughtful feedback. I really 
appreciate your guidance. I'll post v2 of the patch shortly.

Best regards,
Pincheng Wang