Re: [PATCH 2/5] RISC-V: memmove() speed optimized: Replace macros and use fixed-width types

Hi Alexey,

thank you for your comments!

These changes makes newlib ignore "_HAVE_HW_MISALIGNED_ACCESS".


I am happy to change it back to use _HAVE_HW_MISALIGNED_ACCESS if that is
preferred, but since these changes are only to the risc-v port,
and the other functions in this port check for either
__riscv_misaligned_fast, __riscv_misaligned_slow, and/or
__riscv_misaligned_avoid, I figured it
would make sense to opt for one of these macros.
Also, checking for __riscv_misaligned_fast allows for still making use of
the source-alignment-operation if misaligned accesses are allowed but slow.

What purpose of replacing macros? (I can't see advantages to this).
> Is "sizeof(long)" gives something different compared to SZREG ?


The purpose of using SZREG here is to have consistency between all the
functions in the port.
I don't think in this case sizeof(long) would give something different – as
far as I'm aware the valid ABIs for risc-v have long be 4 bytes for a
32-bit architecture,
and 8 bytes for a 64-bit architecture, which matches the respective SZREG
values.

Best,
M

Op di 27 mei 2025 om 08:04 schreef Alexey Lapshin <
[email protected]>:

> These changes makes newlib ignore "_HAVE_HW_MISALIGNED_ACCESS".
>
> What purpose of replacing macros? (I can't see advantages to this).
> Is "sizeof(long)" gives something different compared to SZREG ?
>
> On Mon, 2025-05-26 at 17:05 +0200, m fally wrote:
> > Remove macros or replace them with static inline functions or RISC-V
> > specific macros where applicable.
> > Change data types to fixed-width and/or RISC-V specific types.
> >
> > Reviewed-by: Christian Herber <[email protected]>
> > Signed-off-by: m fally <[email protected]>
> > ---
> >  newlib/libc/machine/riscv/memmove.c | 60 +++++++++++++++++------------
> >  1 file changed, 35 insertions(+), 25 deletions(-)
> >
> > diff --git a/newlib/libc/machine/riscv/memmove.c
> b/newlib/libc/machine/riscv/memmove.c
> > index b48da0905..2e5c6ca9b 100644
> > --- a/newlib/libc/machine/riscv/memmove.c
> > +++ b/newlib/libc/machine/riscv/memmove.c
> > @@ -14,19 +14,29 @@
> >  #else
> >
> >  #include "../../string/local.h"
> > -#include <_ansi.h>
> > +#include "sys/asm.h"
> > +#include "xlenint.h"
> >  #include <limits.h>
> >  #include <stddef.h>
> >  #include <string.h>
> >
> > -/*SUPPRESS 20*/
> > +static inline uint8_t
> > +__libc_fast_xlen_aligned (void *dst, const void *src)
> > +{
> > +#if defined(__riscv_misaligned_fast)
> > +  return 1;
> > +#else
> > +  return !(((uintxlen_t)src & (SZREG - 1)) | ((uintxlen_t)dst & (SZREG
> - 1)));
> > +#endif
> > +}
> > +
> >  void *__inhibit_loop_to_libcall
> >  memmove (void *dst_void, const void *src_void, size_t length)
> >  {
> > -  char *dst = dst_void;
> > -  const char *src = src_void;
> > -  long *aligned_dst;
> > -  const long *aligned_src;
> > +  unsigned char *dst = dst_void;
> > +  const unsigned char *src = src_void;
> > +  uintxlen_t *aligned_dst;
> > +  const uintxlen_t *aligned_src;
> >
> >    if (src < dst && dst < src + length)
> >      {
> > @@ -34,21 +44,21 @@ memmove (void *dst_void, const void *src_void,
> size_t length)
> >        src += length;
> >        dst += length;
> >
> > -      if (!TOO_SMALL_LITTLE_BLOCK (length) && !UNALIGNED_X_Y (src, dst))
> > +      if (length >= SZREG && __libc_fast_xlen_aligned (dst, src))
> >          {
> > -          aligned_dst = (long *)dst;
> > -          aligned_src = (long *)src;
> > +          aligned_dst = (uintxlen_t *)dst;
> > +          aligned_src = (uintxlen_t *)src;
> >
> > -          /* Copy one long word at a time if possible.  */
> > -          while (!TOO_SMALL_LITTLE_BLOCK (length))
> > +          /* Copy one uintxlen_t word at a time if possible.  */
> > +          while (length >= SZREG)
> >              {
> >                *--aligned_dst = *--aligned_src;
> > -              length -= LITTLE_BLOCK_SIZE;
> > +              length -= SZREG;
> >              }
> >
> >            /* Pick up any residual with a byte copier.  */
> > -          dst = (char *)aligned_dst;
> > -          src = (char *)aligned_src;
> > +          dst = (unsigned char *)aligned_dst;
> > +          src = (unsigned char *)aligned_src;
> >          }
> >
> >        while (length--)
> > @@ -61,31 +71,31 @@ memmove (void *dst_void, const void *src_void,
> size_t length)
> >        /* Use optimizing algorithm for a non-destructive copy to closely
> >           match memcpy. If the size is small or either SRC or DST is
> unaligned,
> >           then punt into the byte copy loop.  This should be rare.  */
> > -      if (!TOO_SMALL_LITTLE_BLOCK (length) && !UNALIGNED_X_Y (src, dst))
> > +      if (length >= SZREG && __libc_fast_xlen_aligned (dst, src))
> >          {
> > -          aligned_dst = (long *)dst;
> > -          aligned_src = (long *)src;
> > +          aligned_dst = (uintxlen_t *)dst;
> > +          aligned_src = (uintxlen_t *)src;
> >
> > -          /* Copy 4X long words at a time if possible.  */
> > -          while (!TOO_SMALL_BIG_BLOCK (length))
> > +          /* Copy 4X uintxlen_t words at a time if possible.  */
> > +          while (length >= (SZREG * 4))
> >              {
> >                *aligned_dst++ = *aligned_src++;
> >                *aligned_dst++ = *aligned_src++;
> >                *aligned_dst++ = *aligned_src++;
> >                *aligned_dst++ = *aligned_src++;
> > -              length -= BIG_BLOCK_SIZE;
> > +              length -= SZREG * 4;
> >              }
> >
> > -          /* Copy one long word at a time if possible.  */
> > -          while (!TOO_SMALL_LITTLE_BLOCK (length))
> > +          /* Copy one uintxlen_t word at a time if possible.  */
> > +          while (length >= SZREG)
> >              {
> >                *aligned_dst++ = *aligned_src++;
> > -              length -= LITTLE_BLOCK_SIZE;
> > +              length -= SZREG;
> >              }
> >
> >            /* Pick up any residual with a byte copier.  */
> > -          dst = (char *)aligned_dst;
> > -          src = (char *)aligned_src;
> > +          dst = (unsigned char *)aligned_dst;
> > +          src = (unsigned char *)aligned_src;
> >          }
> >
> >        while (length--)
>
>