Re: [PATCH 2/3] newlib: riscv: Optimize memchr() and memrchr()

On 5/7/25 8:44 AM, Kito Cheng wrote:
>> diff --git a/newlib/libc/machine/riscv/memchr.c b/newlib/libc/machine/riscv/memchr.c
>> index 5c08c12813fe..4202c704e2f3 100644
>> --- a/newlib/libc/machine/riscv/memchr.c
>> +++ b/newlib/libc/machine/riscv/memchr.c
>> @@ -29,10 +29,15 @@ QUICKREF
>>         memchr ansi pure
>>  */
>>
>> -#include <_ansi.h>
>> -#include <string.h>
>> -#include <limits.h>
>> -#include "../../string/local.h"
>> +#include <sys/asm.h>
>> +#include <stddef.h>
>> +#include "rv_string.h"
>> +
>> +#if __riscv_zilsd
>> +#undef  SZREG
>> +#define SZREG           8
> 
> Could you define a MOVE_UNIT rather than SZREG here? It's kinda confusing.

Sure, I'll change the name.

>>
>> -      src = (unsigned char *) asrc;
>> +  if (src < end_addr)
>> +    {
>> +      uintxlen_t mask = __libc_splat_byte(d);
>> +      uintlslen_t val;
>> +
>> +      do
>> +        {
>> +#if __riscv_zilsd
>> +          asm volatile ("ld %0, 0(%1)"
>> +                        : "=R" (val)
>> +                        : "r" (src)
>> +          );
> 
> I strongly prefer not to use inline asm here, let the compiler do
> that, although I know upstream GCC doesn't implement that yet...

So the problem is that there's currently no way to take advantage of it
without inline assembly. Clang added it recently, but I'm not aware of
any way of using it besides assembly (I don't think they have an
intrinsic function defined).

I'd be more than happy to let the compiler handle it so inline assembly
doesn't have to be used, but at the same time, I'm not sure it's going
to work without at least an intrinsic existing. I don't know how else
you'd tell the compiler to use that specific instruction like you do for
the bit instructions.

>> diff --git a/newlib/libc/machine/riscv/memrchr.c b/newlib/libc/machine/riscv/memrchr.c
>> index 8d15ccb780ec..a67c2fefe124 100644
>> --- a/newlib/libc/machine/riscv/memrchr.c
>> +++ b/newlib/libc/machine/riscv/memrchr.c
>> @@ -29,61 +29,141 @@ QUICKREF
>>         memrchr
>>  */
>>
>> -#include <_ansi.h>
>> -#include <string.h>
>> -#include <limits.h>
>> -#include "../../string/local.h"
>> +#include <sys/asm.h>
>> +#include <stddef.h>
>> +#include "rv_string.h"
>> +
>> +#if __riscv_zilsd
>> +#undef  SZREG
>> +#define SZREG           8
> 
> Same here

I'll change this as well.