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

In the commit you mentioned, several RISC-V files were updated with the macros that are also used in this patch.
Can you be specific which functionality you believe will be broken?

Christian

> -----Original Message-----
> From: Alexey Lapshin <[email protected]>
> Sent: Tuesday, 27 May 2025 14:54
> To: [email protected]
> Cc: [email protected]; Christian Herber (OSS)
> <[email protected]>
> Subject: Re: [PATCH 2/5] RISC-V: memmove() speed optimized: Replace
> macros and use fixed-width types
> 
> Please take a look at this commit:
> https://github.com/bminor/newlib/commit/dcf5d237fd84bec7f202cf4180362
> 08b5c9901c9
> 
> I would like to have --enable-newlib-hw-misaligned-access option in working
> condition. Please don't break the behavior
> 
> On Tue, 2025-05-27 at 13:32 +0200, Marlene Fally wrote:
> >
> > [External: This email originated outside Espressif]
> >
> >
> > 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--)
> > >
> > >