| Message ID | 20210617152754.17960-4-mcroce@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: optimized mem* functions | expand |
Looks nice, except IS_ENABLED would be useful here again, as well as a placement in lib/.
Στις 2021-06-17 18:27, Matteo Croce έγραψε: > + > +void *__memset(void *s, int c, size_t count) > +{ > + union types dest = { .u8 = s }; > + > + if (count >= MIN_THRESHOLD) { > + const int bytes_long = BITS_PER_LONG / 8; You could make 'const int bytes_long = BITS_PER_LONG / 8;' and 'const int mask = bytes_long - 1;' from your memcpy patch visible to memset as well (static const...) and use them here (mask would make more sense to be named as word_mask). > + unsigned long cu = (unsigned long)c; > + > + /* Compose an ulong with 'c' repeated 4/8 times */ > + cu |= cu << 8; > + cu |= cu << 16; > +#if BITS_PER_LONG == 64 > + cu |= cu << 32; > +#endif > + You don't have to create cu here, you'll fill dest buffer with 'c' anyway so after filling up enough 'c's to be able to grab an aligned word full of them from dest, you can just grab that word and keep filling up dest with it. > +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS > + /* Fill the buffer one byte at time until the destination > + * is aligned on a 32/64 bit boundary. > + */ > + for (; count && dest.uptr % bytes_long; count--) You could reuse & mask here instead of % bytes_long. > + *dest.u8++ = c; > +#endif I noticed you also used CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS on your memcpy patch, is it worth it here ? To begin with riscv doesn't set it and even if it did we are talking about a loop that will run just a few times to reach the alignment boundary (worst case scenario it'll run 7 times), I don't think we gain much here, even for archs that have efficient unaligned access.
From: Nick Kossifidis > Sent: 22 June 2021 02:08 > > Στις 2021-06-17 18:27, Matteo Croce έγραψε: > > + > > +void *__memset(void *s, int c, size_t count) > > +{ > > + union types dest = { .u8 = s }; > > + > > + if (count >= MIN_THRESHOLD) { > > + const int bytes_long = BITS_PER_LONG / 8; > > You could make 'const int bytes_long = BITS_PER_LONG / 8;' What is wrong with sizeof (long) ? ... > > + unsigned long cu = (unsigned long)c; > > + > > + /* Compose an ulong with 'c' repeated 4/8 times */ > > + cu |= cu << 8; > > + cu |= cu << 16; > > +#if BITS_PER_LONG == 64 > > + cu |= cu << 32; > > +#endif > > + > > You don't have to create cu here, you'll fill dest buffer with 'c' > anyway so after filling up enough 'c's to be able to grab an aligned > word full of them from dest, you can just grab that word and keep > filling up dest with it. That will be a lot slower - especially if run on something like x86. A write-read of the same size is optimised by the store-load forwarder. But the byte write, word read will have to go via the cache. You can just write: cu = (unsigned long)c * 0x0101010101010101ull; and let the compiler sort out the best way to generate the constant. > > > +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS > > + /* Fill the buffer one byte at time until the destination > > + * is aligned on a 32/64 bit boundary. > > + */ > > + for (; count && dest.uptr % bytes_long; count--) > > You could reuse & mask here instead of % bytes_long. > > > + *dest.u8++ = c; > > +#endif > > I noticed you also used CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS on your > memcpy patch, is it worth it here ? To begin with riscv doesn't set it > and even if it did we are talking about a loop that will run just a few > times to reach the alignment boundary (worst case scenario it'll run 7 > times), I don't think we gain much here, even for archs that have > efficient unaligned access. With CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS it probably isn't worth even checking the alignment. While aligning the copy will be quicker for an unaligned buffer they almost certainly don't happen often enough to worry about. In any case you'd want to do a misaligned word write to the start of the buffer - not separate byte writes. Provided the buffer is long enough you can also do a misaligned write to the end of the buffer before filling from the start. I suspect you may need either barrier() or use a ptr to packed to avoid the perverted 'undefined behaviour' fubar.' David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Jun 22, 2021 at 3:07 AM Nick Kossifidis <mick@ics.forth.gr> wrote: > > Στις 2021-06-17 18:27, Matteo Croce έγραψε: > > + > > +void *__memset(void *s, int c, size_t count) > > +{ > > + union types dest = { .u8 = s }; > > + > > + if (count >= MIN_THRESHOLD) { > > + const int bytes_long = BITS_PER_LONG / 8; > > You could make 'const int bytes_long = BITS_PER_LONG / 8;' and 'const > int mask = bytes_long - 1;' from your memcpy patch visible to memset as > well (static const...) and use them here (mask would make more sense to > be named as word_mask). > I'll do > > + unsigned long cu = (unsigned long)c; > > + > > + /* Compose an ulong with 'c' repeated 4/8 times */ > > + cu |= cu << 8; > > + cu |= cu << 16; > > +#if BITS_PER_LONG == 64 > > + cu |= cu << 32; > > +#endif > > + > > You don't have to create cu here, you'll fill dest buffer with 'c' > anyway so after filling up enough 'c's to be able to grab an aligned > word full of them from dest, you can just grab that word and keep > filling up dest with it. > I tried that, but this way I have to wait 8 bytes more before starting the memset. And, the machine code needed to generate 'cu' is just 6 instructions on riscv: slli a5,a0,8 or a5,a5,a0 slli a0,a5,16 or a0,a0,a5 slli a5,a0,32 or a0,a5,a0 so probably it's not worth it. > > +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS > > + /* Fill the buffer one byte at time until the destination > > + * is aligned on a 32/64 bit boundary. > > + */ > > + for (; count && dest.uptr % bytes_long; count--) > > You could reuse & mask here instead of % bytes_long. > Sure, even if the machine code will be the same. > > + *dest.u8++ = c; > > +#endif > > I noticed you also used CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS on your > memcpy patch, is it worth it here ? To begin with riscv doesn't set it > and even if it did we are talking about a loop that will run just a few > times to reach the alignment boundary (worst case scenario it'll run 7 > times), I don't think we gain much here, even for archs that have > efficient unaligned access. It doesn't _now_, but maybe in the future we will have a CPU which handles unaligned accesses correctly!
On Tue, Jun 22, 2021 at 10:38 AM David Laight <David.Laight@aculab.com> wrote: > > From: Nick Kossifidis > > Sent: 22 June 2021 02:08 > > > > Στις 2021-06-17 18:27, Matteo Croce έγραψε: > > > + > > > +void *__memset(void *s, int c, size_t count) > > > +{ > > > + union types dest = { .u8 = s }; > > > + > > > + if (count >= MIN_THRESHOLD) { > > > + const int bytes_long = BITS_PER_LONG / 8; > > > > You could make 'const int bytes_long = BITS_PER_LONG / 8;' > > What is wrong with sizeof (long) ? > ... Nothing, I guess that BITS_PER_LONG is just (sizeof(long) * 8) anyway > > > + unsigned long cu = (unsigned long)c; > > > + > > > + /* Compose an ulong with 'c' repeated 4/8 times */ > > > + cu |= cu << 8; > > > + cu |= cu << 16; > > > +#if BITS_PER_LONG == 64 > > > + cu |= cu << 32; > > > +#endif > > > + > > > > You don't have to create cu here, you'll fill dest buffer with 'c' > > anyway so after filling up enough 'c's to be able to grab an aligned > > word full of them from dest, you can just grab that word and keep > > filling up dest with it. > > That will be a lot slower - especially if run on something like x86. > A write-read of the same size is optimised by the store-load forwarder. > But the byte write, word read will have to go via the cache. > > You can just write: > cu = (unsigned long)c * 0x0101010101010101ull; > and let the compiler sort out the best way to generate the constant. > Interesting. I see that most compilers do an integer multiplication, is it faster than three shift and three or? clang on riscv generates even more instructions to create the immediate: unsigned long repeat_shift(int c) { unsigned long cu = (unsigned long)c; cu |= cu << 8; cu |= cu << 16; cu |= cu << 32; return cu; } unsigned long repeat_mul(int c) { return (unsigned long)c * 0x0101010101010101ull; } repeat_shift: slli a1, a0, 8 or a0, a0, a1 slli a1, a0, 16 or a0, a0, a1 slli a1, a0, 32 or a0, a0, a1 ret repeat_mul: lui a1, 4112 addiw a1, a1, 257 slli a1, a1, 16 addi a1, a1, 257 slli a1, a1, 16 addi a1, a1, 257 mul a0, a0, a1 ret > > > > > +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS > > > + /* Fill the buffer one byte at time until the destination > > > + * is aligned on a 32/64 bit boundary. > > > + */ > > > + for (; count && dest.uptr % bytes_long; count--) > > > > You could reuse & mask here instead of % bytes_long. > > > > > + *dest.u8++ = c; > > > +#endif > > > > I noticed you also used CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS on your > > memcpy patch, is it worth it here ? To begin with riscv doesn't set it > > and even if it did we are talking about a loop that will run just a few > > times to reach the alignment boundary (worst case scenario it'll run 7 > > times), I don't think we gain much here, even for archs that have > > efficient unaligned access. > > With CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS it probably isn't worth > even checking the alignment. > While aligning the copy will be quicker for an unaligned buffer they > almost certainly don't happen often enough to worry about. > In any case you'd want to do a misaligned word write to the start > of the buffer - not separate byte writes. > Provided the buffer is long enough you can also do a misaligned write > to the end of the buffer before filling from the start. > I don't understand this one, a misaligned write here is ~30x slower than an aligned one because it gets trapped and emulated in SBI. How can this be convenient? > I suspect you may need either barrier() or use a ptr to packed > to avoid the perverted 'undefined behaviour' fubar.' > Which UB are you referring to? Regards, -- per aspera ad upstream
From: Matteo Croce > Sent: 23 June 2021 02:15 > > On Tue, Jun 22, 2021 at 10:38 AM David Laight <David.Laight@aculab.com> wrote: > > > > From: Nick Kossifidis ... > > You can just write: > > cu = (unsigned long)c * 0x0101010101010101ull; > > and let the compiler sort out the best way to generate the constant. > > > > Interesting. I see that most compilers do an integer multiplication, > is it faster than three shift and three or? > > clang on riscv generates even more instructions to create the immediate: > > unsigned long repeat_shift(int c) > { > unsigned long cu = (unsigned long)c; > cu |= cu << 8; > cu |= cu << 16; > cu |= cu << 32; > > return cu; > } > > unsigned long repeat_mul(int c) > { > return (unsigned long)c * 0x0101010101010101ull; > } > > repeat_shift: > slli a1, a0, 8 > or a0, a0, a1 > slli a1, a0, 16 > or a0, a0, a1 > slli a1, a0, 32 > or a0, a0, a1 > ret > > repeat_mul: > lui a1, 4112 > addiw a1, a1, 257 > slli a1, a1, 16 > addi a1, a1, 257 > slli a1, a1, 16 > addi a1, a1, 257 > mul a0, a0, a1 > ret Hmmm... I expected the compiler to convert it to the first form. It is also pretty crap at generating that constant. Stupid compilers. In any case, for the usual case of 'c' being a constant zero you really don't want the latency of those instructions at all. It is almost worth just pushing that expansion into the caller. eg by having: #define memset(p, v, l) memset_w(p, (v) * 0x0101010101010101, l) (or some other byte replicator). Really annoyingly you want to write the code that generates the 64bit constant, and then have the compiler optimise away the part that generates the high 32 bits on 32 bits systems. But one of the compilers is going to 'bleat' about truncating a constant value. Stupid compilers (again). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/arch/riscv/include/asm/string.h b/arch/riscv/include/asm/string.h index 25d9b9078569..90500635035a 100644 --- a/arch/riscv/include/asm/string.h +++ b/arch/riscv/include/asm/string.h @@ -6,14 +6,10 @@ #ifndef _ASM_RISCV_STRING_H #define _ASM_RISCV_STRING_H -#include <linux/types.h> -#include <linux/linkage.h> - -#define __HAVE_ARCH_MEMSET -extern asmlinkage void *memset(void *, int, size_t); -extern asmlinkage void *__memset(void *, int, size_t); - #ifdef CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE +#define __HAVE_ARCH_MEMSET +extern void *memset(void *s, int c, size_t count); +extern void *__memset(void *s, int c, size_t count); #define __HAVE_ARCH_MEMCPY extern void *memcpy(void *dest, const void *src, size_t count); extern void *__memcpy(void *dest, const void *src, size_t count); diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index d3081e4d9600..e635ce1e5645 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -31,7 +31,6 @@ obj-y += syscall_table.o obj-y += sys_riscv.o obj-y += time.o obj-y += traps.o -obj-y += riscv_ksyms.o obj-y += stacktrace.o obj-y += cacheinfo.o obj-y += patch.o diff --git a/arch/riscv/kernel/riscv_ksyms.c b/arch/riscv/kernel/riscv_ksyms.c deleted file mode 100644 index 361565c4db7e..000000000000 --- a/arch/riscv/kernel/riscv_ksyms.c +++ /dev/null @@ -1,13 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * Copyright (C) 2017 Zihao Yu - */ - -#include <linux/export.h> -#include <linux/uaccess.h> - -/* - * Assembly functions that may be used (directly or indirectly) by modules - */ -EXPORT_SYMBOL(memset); -EXPORT_SYMBOL(__memset); diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile index 484f5ff7b508..e33263cc622a 100644 --- a/arch/riscv/lib/Makefile +++ b/arch/riscv/lib/Makefile @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0-only lib-y += delay.o -lib-y += memset.o lib-$(CONFIG_MMU) += uaccess.o lib-$(CONFIG_64BIT) += tishift.o lib-$(CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE) += string.o diff --git a/arch/riscv/lib/memset.S b/arch/riscv/lib/memset.S deleted file mode 100644 index 34c5360c6705..000000000000 --- a/arch/riscv/lib/memset.S +++ /dev/null @@ -1,113 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -/* - * Copyright (C) 2013 Regents of the University of California - */ - - -#include <linux/linkage.h> -#include <asm/asm.h> - -/* void *memset(void *, int, size_t) */ -ENTRY(__memset) -WEAK(memset) - move t0, a0 /* Preserve return value */ - - /* Defer to byte-oriented fill for small sizes */ - sltiu a3, a2, 16 - bnez a3, 4f - - /* - * Round to nearest XLEN-aligned address - * greater than or equal to start address - */ - addi a3, t0, SZREG-1 - andi a3, a3, ~(SZREG-1) - beq a3, t0, 2f /* Skip if already aligned */ - /* Handle initial misalignment */ - sub a4, a3, t0 -1: - sb a1, 0(t0) - addi t0, t0, 1 - bltu t0, a3, 1b - sub a2, a2, a4 /* Update count */ - -2: /* Duff's device with 32 XLEN stores per iteration */ - /* Broadcast value into all bytes */ - andi a1, a1, 0xff - slli a3, a1, 8 - or a1, a3, a1 - slli a3, a1, 16 - or a1, a3, a1 -#ifdef CONFIG_64BIT - slli a3, a1, 32 - or a1, a3, a1 -#endif - - /* Calculate end address */ - andi a4, a2, ~(SZREG-1) - add a3, t0, a4 - - andi a4, a4, 31*SZREG /* Calculate remainder */ - beqz a4, 3f /* Shortcut if no remainder */ - neg a4, a4 - addi a4, a4, 32*SZREG /* Calculate initial offset */ - - /* Adjust start address with offset */ - sub t0, t0, a4 - - /* Jump into loop body */ - /* Assumes 32-bit instruction lengths */ - la a5, 3f -#ifdef CONFIG_64BIT - srli a4, a4, 1 -#endif - add a5, a5, a4 - jr a5 -3: - REG_S a1, 0(t0) - REG_S a1, SZREG(t0) - REG_S a1, 2*SZREG(t0) - REG_S a1, 3*SZREG(t0) - REG_S a1, 4*SZREG(t0) - REG_S a1, 5*SZREG(t0) - REG_S a1, 6*SZREG(t0) - REG_S a1, 7*SZREG(t0) - REG_S a1, 8*SZREG(t0) - REG_S a1, 9*SZREG(t0) - REG_S a1, 10*SZREG(t0) - REG_S a1, 11*SZREG(t0) - REG_S a1, 12*SZREG(t0) - REG_S a1, 13*SZREG(t0) - REG_S a1, 14*SZREG(t0) - REG_S a1, 15*SZREG(t0) - REG_S a1, 16*SZREG(t0) - REG_S a1, 17*SZREG(t0) - REG_S a1, 18*SZREG(t0) - REG_S a1, 19*SZREG(t0) - REG_S a1, 20*SZREG(t0) - REG_S a1, 21*SZREG(t0) - REG_S a1, 22*SZREG(t0) - REG_S a1, 23*SZREG(t0) - REG_S a1, 24*SZREG(t0) - REG_S a1, 25*SZREG(t0) - REG_S a1, 26*SZREG(t0) - REG_S a1, 27*SZREG(t0) - REG_S a1, 28*SZREG(t0) - REG_S a1, 29*SZREG(t0) - REG_S a1, 30*SZREG(t0) - REG_S a1, 31*SZREG(t0) - addi t0, t0, 32*SZREG - bltu t0, a3, 3b - andi a2, a2, SZREG-1 /* Update count */ - -4: - /* Handle trailing misalignment */ - beqz a2, 6f - add a3, t0, a2 -5: - sb a1, 0(t0) - addi t0, t0, 1 - bltu t0, a3, 5b -6: - ret -END(__memset) diff --git a/arch/riscv/lib/string.c b/arch/riscv/lib/string.c index 9c7009d43c39..1fb4de351516 100644 --- a/arch/riscv/lib/string.c +++ b/arch/riscv/lib/string.c @@ -112,3 +112,42 @@ EXPORT_SYMBOL(__memmove); void *memmove(void *dest, const void *src, size_t count) __weak __alias(__memmove); EXPORT_SYMBOL(memmove); + +void *__memset(void *s, int c, size_t count) +{ + union types dest = { .u8 = s }; + + if (count >= MIN_THRESHOLD) { + const int bytes_long = BITS_PER_LONG / 8; + unsigned long cu = (unsigned long)c; + + /* Compose an ulong with 'c' repeated 4/8 times */ + cu |= cu << 8; + cu |= cu << 16; +#if BITS_PER_LONG == 64 + cu |= cu << 32; +#endif + +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS + /* Fill the buffer one byte at time until the destination + * is aligned on a 32/64 bit boundary. + */ + for (; count && dest.uptr % bytes_long; count--) + *dest.u8++ = c; +#endif + + /* Copy using the largest size allowed */ + for (; count >= bytes_long; count -= bytes_long) + *dest.ulong++ = cu; + } + + /* copy the remainder */ + while (count--) + *dest.u8++ = c; + + return s; +} +EXPORT_SYMBOL(__memset); + +void *memset(void *s, int c, size_t count) __weak __alias(__memset); +EXPORT_SYMBOL(memset);