* [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization @ 2023-12-15 8:31 Xiao Zeng 2023-12-15 10:28 ` Torbjorn SVENSSON 0 siblings, 1 reply; 12+ messages in thread From: Xiao Zeng @ 2023-12-15 8:31 UTC (permalink / raw) To: newlib; +Cc: vapier, palmer, jeffreyalaw, Xiao Zeng Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com> --- newlib/libc/string/strcspn.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/newlib/libc/string/strcspn.c b/newlib/libc/string/strcspn.c index abaa93ad6..8ac0bf10c 100644 --- a/newlib/libc/string/strcspn.c +++ b/newlib/libc/string/strcspn.c @@ -37,12 +37,10 @@ strcspn (const char *s1, for (c = s2; *c; c++) { if (*s1 == *c) - break; + goto end; } - if (*c) - break; s1++; } - +end: return s1 - s; } -- 2.17.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-15 8:31 [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization Xiao Zeng @ 2023-12-15 10:28 ` Torbjorn SVENSSON 2023-12-16 4:45 ` Xiao Zeng 2023-12-16 9:30 ` Xiao Zeng 0 siblings, 2 replies; 12+ messages in thread From: Torbjorn SVENSSON @ 2023-12-15 10:28 UTC (permalink / raw) To: Xiao Zeng, newlib; +Cc: vapier, palmer, jeffreyalaw Hello Xiao, On 2023-12-15 09:31, Xiao Zeng wrote: > Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com> > --- > newlib/libc/string/strcspn.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/newlib/libc/string/strcspn.c b/newlib/libc/string/strcspn.c > index abaa93ad6..8ac0bf10c 100644 > --- a/newlib/libc/string/strcspn.c > +++ b/newlib/libc/string/strcspn.c > @@ -37,12 +37,10 @@ strcspn (const char *s1, > for (c = s2; *c; c++) > { > if (*s1 == *c) > - break; > + goto end; > } > - if (*c) > - break; > s1++; > } > - > +end: > return s1 - s; > } Just looking at this small snippet of code, I would say that the previous code and your suggestion won't do the same thing. Do you have unit tests that confirm that the behavior is identical with the current implementation and your suggested change? When I run your suggestion, I get return value 0, but with the current implementation it's 3 for this call: strspn("129th", "1234567890"). Kind regards, Torbjörn ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-15 10:28 ` Torbjorn SVENSSON @ 2023-12-16 4:45 ` Xiao Zeng 2023-12-16 4:56 ` Xiao Zeng 2023-12-20 15:08 ` Torbjorn SVENSSON 2023-12-16 9:30 ` Xiao Zeng 1 sibling, 2 replies; 12+ messages in thread From: Xiao Zeng @ 2023-12-16 4:45 UTC (permalink / raw) To: Torbjorn SVENSSON, newlib; +Cc: vapier, palmer, jeffreyalaw 2023-12-15 18:28 Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> wrote: > Hi Bob, I worked under the risc-v architecture and obtained the newlib library through cross compilation. This is my first attempt to contribute code to newlib. >Hello Xiao, > >On 2023-12-15 09:31, Xiao Zeng wrote: >> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com> >> --- >> newlib/libc/string/strcspn.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/newlib/libc/string/strcspn.c b/newlib/libc/string/strcspn.c >> index abaa93ad6..8ac0bf10c 100644 >> --- a/newlib/libc/string/strcspn.c >> +++ b/newlib/libc/string/strcspn.c >> @@ -37,12 +37,10 @@ strcspn (const char *s1, >> for (c = s2; *c; c++) >> { >> if (*s1 == *c) >> - break; >> + goto end; >> } >> - if (*c) >> - break; >> s1++; >> } >> - >> +end: >> return s1 - s; >> } > >Just looking at this small snippet of code, I would say that the >previous code and your suggestion won't do the same thing. > >Do you have unit tests that confirm that the behavior is identical with >the current implementation and your suggested change? 1 I must admit that I did not conduct a complete newlib regression test. Anyway, I should apologize for this. After completing cross compilation, I tried: ----------------- make check ----------------- Received the following error message: -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- Target is riscv64-unknown-elf Host is x86_64-pc-linux-gnu ... The error code is TCL LOOKUP COMMAND newlib_target_compile The info on the error is: invalid command name "newlib_target_compile" while executing "::tcl_unknown newlib_target_compile /home/user/Downloads/tools/tools.git/newlib.git/newlib/testsuite/newlib.iconv/iconvnm.c /home/user/Downloads/tools..." ("uplevel" body line 1) invoked from within "uplevel 1 ::tcl_unknown $args" -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- I know the reason for the error: Target and Host do not match, which is a common issue in cross compilation. I have looked up some information, but I have not found a solution. 2 On the basis of the above, I searched for test cases of the strcspn function in newlib: ------------------------------------------------------------------------------------------------- newlib/libm/test/string.c:311: check(strcspn("abcba", "qx") == 5); /* Whole string. */ newlib/libm/test/string.c:312: check(strcspn("abcba", "cx") == 2); /* Partial. */ newlib/libm/test/string.c:313: check(strcspn("abc", "abc") == 0); /* None. */ newlib/libm/test/string.c:314: check(strcspn("", "ab") == 0); /* Null string. */ newlib/libm/test/string.c:315: check(strcspn("abc", "") == 3); /* Null search list. */ ------------------------------------------------------------------------------------------------- I simply believe that these 5 test cases are all the test cases related to strcspn in newlib. After local testing, all these test cases can pass in my patch. > >When I run your suggestion, I get return value 0, but with the current >implementation it's 3 for this call: strspn("129th", "1234567890"). 3 It is worth noting that strcspn and strspn only differ by one letter 'c'. I also often confuse them -:), maybe you are the same. > >Kind regards, >Torbjörn 4 Perhaps someone could point out how to test newlib under the risc-v architecture, and I would greatly appreciate it. Of course, there are also great methods for testing newlib under other architectures. 5 By the way, it is not possible to compile the latest newlib on host (x86_64-pc-linux-gnu) (compilation quickly ended as if nothing had happened), even if ../configure comes with the parameter -- with-newlib. Thanks Xiao Zeng ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-16 4:45 ` Xiao Zeng @ 2023-12-16 4:56 ` Xiao Zeng 2023-12-20 15:08 ` Torbjorn SVENSSON 1 sibling, 0 replies; 12+ messages in thread From: Xiao Zeng @ 2023-12-16 4:56 UTC (permalink / raw) To: Torbjorn SVENSSON, newlib; +Cc: vapier, palmer, jeffreyalaw 2023-12-16 12:45 Xiao Zeng <zengxiao@eswincomputing.com> wrote: > >2023-12-15 18:28 Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> wrote: >> >Hi Bob, I worked under the risc-v architecture and obtained the newlib library through Sorry, Torbjorn, I accidentally wrote your name wrong. I apologize for my carelessness. >cross compilation. This is my first attempt to contribute code to newlib. > >>Hello Xiao, >> >>On 2023-12-15 09:31, Xiao Zeng wrote: >>> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com> >>> --- >>> newlib/libc/string/strcspn.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/newlib/libc/string/strcspn.c b/newlib/libc/string/strcspn.c >>> index abaa93ad6..8ac0bf10c 100644 >>> --- a/newlib/libc/string/strcspn.c >>> +++ b/newlib/libc/string/strcspn.c >>> @@ -37,12 +37,10 @@ strcspn (const char *s1, >>> for (c = s2; *c; c++) >>> { >>> if (*s1 == *c) >>> - break; >>> + goto end; >>> } >>> - if (*c) >>> - break; >>> s1++; >>> } >>> - >>> +end: >>> return s1 - s; >>> } >> >>Just looking at this small snippet of code, I would say that the >>previous code and your suggestion won't do the same thing. >> >>Do you have unit tests that confirm that the behavior is identical with >>the current implementation and your suggested change? >1 I must admit that I did not conduct a complete newlib regression test. >Anyway, I should apologize for this. After completing cross compilation, I tried: >----------------- >make check >----------------- > >Received the following error message: >-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- >Target is riscv64-unknown-elf >Host is x86_64-pc-linux-gnu >... >The error code is TCL LOOKUP COMMAND newlib_target_compile >The info on the error is: >invalid command name "newlib_target_compile" > while executing >"::tcl_unknown newlib_target_compile /home/user/Downloads/tools/tools.git/newlib.git/newlib/testsuite/newlib.iconv/iconvnm.c /home/user/Downloads/tools..." > ("uplevel" body line 1) > invoked from within >"uplevel 1 ::tcl_unknown $args" >-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > >I know the reason for the error: Target and Host do not match, which >is a common issue in cross compilation. I have looked up some >information, but I have not found a solution. > >2 On the basis of the above, I searched for test cases of the strcspn >function in newlib: >------------------------------------------------------------------------------------------------- >newlib/libm/test/string.c:311: check(strcspn("abcba", "qx") == 5); /* Whole string. */ >newlib/libm/test/string.c:312: check(strcspn("abcba", "cx") == 2); /* Partial. */ >newlib/libm/test/string.c:313: check(strcspn("abc", "abc") == 0); /* None. */ >newlib/libm/test/string.c:314: check(strcspn("", "ab") == 0); /* Null string. */ >newlib/libm/test/string.c:315: check(strcspn("abc", "") == 3); /* Null search list. */ >------------------------------------------------------------------------------------------------- > >I simply believe that these 5 test cases are all the test cases related to >strcspn in newlib. After local testing, all these test cases can pass in my patch. >> >>When I run your suggestion, I get return value 0, but with the current >>implementation it's 3 for this call: strspn("129th", "1234567890"). >3 It is worth noting that strcspn and strspn only differ by one letter 'c'. I also >often confuse them -:), maybe you are the same. > >> >>Kind regards, >>Torbjörn > >4 Perhaps someone could point out how to test newlib under the risc-v >architecture, and I would greatly appreciate it. Of course, there are also >great methods for testing newlib under other architectures. > >5 By the way, it is not possible to compile the latest newlib on host (x86_64-pc-linux-gnu) >(compilation quickly ended as if nothing had happened), even if ../configure >comes with the parameter -- with-newlib. > > >Thanks >Xiao Zeng > Thanks Xiao Zeng ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-16 4:45 ` Xiao Zeng 2023-12-16 4:56 ` Xiao Zeng @ 2023-12-20 15:08 ` Torbjorn SVENSSON 2023-12-21 1:22 ` Xiao Zeng 1 sibling, 1 reply; 12+ messages in thread From: Torbjorn SVENSSON @ 2023-12-20 15:08 UTC (permalink / raw) To: Xiao Zeng, newlib; +Cc: vapier, palmer, jeffreyalaw Hi Xiao, On 2023-12-16 05:45, Xiao Zeng wrote: > 3 It is worth noting that strcspn and strspn only differ by one letter 'c'. I also > often confuse them -:), maybe you are the same. Yes, sorry, I did that mistake. Again, sorry for the noise. :'( Kind regards, Torbjörn ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-20 15:08 ` Torbjorn SVENSSON @ 2023-12-21 1:22 ` Xiao Zeng 0 siblings, 0 replies; 12+ messages in thread From: Xiao Zeng @ 2023-12-21 1:22 UTC (permalink / raw) To: Torbjorn SVENSSON, newlib; +Cc: vapier, palmer, jeffreyalaw 2023-12-20 23:08 Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> wrote: > >Hi Xiao, > >On 2023-12-16 05:45, Xiao Zeng wrote: >> 3 It is worth noting that strcspn and strspn only differ by one letter 'c'. I also >> often confuse them -:), maybe you are the same. > >Yes, sorry, I did that mistake. It's ok. I am very pleased to receive your code review comments in the upcoming patchs, which will bring great encouragement to me as a newcomer. At the same time, it can also create better code, which is why we come together. >Again, sorry for the noise. :'( > > >Kind regards, >Torbjörn Thanks Xiao Zeng ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-15 10:28 ` Torbjorn SVENSSON 2023-12-16 4:45 ` Xiao Zeng @ 2023-12-16 9:30 ` Xiao Zeng 2023-12-20 4:24 ` Jeff Johnston 1 sibling, 1 reply; 12+ messages in thread From: Xiao Zeng @ 2023-12-16 9:30 UTC (permalink / raw) To: Torbjorn SVENSSON, newlib; +Cc: vapier, palmer, jeffreyalaw 2023-12-15 18:28 Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> wrote: > >Hello Xiao, > >On 2023-12-15 09:31, Xiao Zeng wrote: >> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com> >> --- >> newlib/libc/string/strcspn.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/newlib/libc/string/strcspn.c b/newlib/libc/string/strcspn.c >> index abaa93ad6..8ac0bf10c 100644 >> --- a/newlib/libc/string/strcspn.c >> +++ b/newlib/libc/string/strcspn.c >> @@ -37,12 +37,10 @@ strcspn (const char *s1, >> for (c = s2; *c; c++) >> { >> if (*s1 == *c) >> - break; >> + goto end; >> } >> - if (*c) >> - break; >> s1++; >> } >> - >> +end: >> return s1 - s; >> } > >Just looking at this small snippet of code, I would say that the >previous code and your suggestion won't do the same thing. > >Do you have unit tests that confirm that the behavior is identical with >the current implementation and your suggested change? > >When I run your suggestion, I get return value 0, but with the current >implementation it's 3 for this call: strspn("129th", "1234567890"). > >Kind regards, >Torbjörn After applying this patch, provide a comparison of assembly code under the risc-v architecture, with default compilation parameters used in both of them: no-patch: --------------------------------------------------------- libc_a-strcspn.o: file format elf64-littleriscv Disassembly of section .text: 0000000000000000 <strcspn>: 0: 00054683 lbu a3,0(a0) 4: 06068263 beqz a3,68 <.L9> 8: 0005c803 lbu a6,0(a1) c: 00050613 mv a2,a0 0000000000000010 <.LVL1>: 10: 02080463 beqz a6,38 <.L14> 0000000000000014 <.L6>: 14: 00058793 mv a5,a1 18: 00080713 mv a4,a6 1c: 00c0006f j 28 <.L5> 0000000000000020 <.L4>: 20: 0007c703 lbu a4,0(a5) 24: 02070863 beqz a4,54 <.L17> 0000000000000028 <.L5>: 28: 00178793 addi a5,a5,1 000000000000002c <.LVL5>: 2c: fee69ae3 bne a3,a4,20 <.L4> 0000000000000030 <.L7>: 30: 40a60533 sub a0,a2,a0 0000000000000034 <.LVL7>: 34: 00008067 ret 0000000000000038 <.L14>: 38: 00164683 lbu a3,1(a2) 3c: 00160613 addi a2,a2,1 40: fe0688e3 beqz a3,30 <.L7> 44: 00164683 lbu a3,1(a2) 48: 00160613 addi a2,a2,1 4c: fe0696e3 bnez a3,38 <.L14> 50: fe1ff06f j 30 <.L7> 0000000000000054 <.L17>: 54: 00164683 lbu a3,1(a2) 58: 00160613 addi a2,a2,1 5c: fa069ce3 bnez a3,14 <.L6> 60: 40a60533 sub a0,a2,a0 0000000000000064 <.LVL13>: 64: 00008067 ret 0000000000000068 <.L9>: 68: 00000513 li a0,0 000000000000006c <.LVL15>: 6c: 00008067 ret --------------------------------------------------------- patch --------------------------------------------------------- libc_a-strcspn.o: file format elf64-littleriscv Disassembly of section .text: 0000000000000000 <strcspn>: 0: 00054683 lbu a3,0(a0) 4: 04068a63 beqz a3,58 <.L2> 8: 0005c803 lbu a6,0(a1) c: 00050613 mv a2,a0 0000000000000010 <.LVL1>: 10: 02080c63 beqz a6,48 <.L14> 0000000000000014 <.L6>: 14: 00058793 mv a5,a1 18: 00080713 mv a4,a6 1c: 00c0006f j 28 <.L5> 0000000000000020 <.L4>: 20: 0007c703 lbu a4,0(a5) 24: 00070a63 beqz a4,38 <.L16> 0000000000000028 <.L5>: 28: 00178793 addi a5,a5,1 000000000000002c <.LVL5>: 2c: fee69ae3 bne a3,a4,20 <.L4> 0000000000000030 <.L7>: 30: 40a60533 sub a0,a2,a0 0000000000000034 <.LVL7>: 34: 00008067 ret 0000000000000038 <.L16>: 38: 00164683 lbu a3,1(a2) 3c: 00160613 addi a2,a2,1 40: fc069ae3 bnez a3,14 <.L6> 44: fedff06f j 30 <.L7> 0000000000000048 <.L14>: 48: 00164683 lbu a3,1(a2) 4c: 00160613 addi a2,a2,1 50: fe069ce3 bnez a3,48 <.L14> 54: fddff06f j 30 <.L7> 0000000000000058 <.L2>: 58: 00000513 li a0,0 000000000000005c <.LVL12>: 5c: 00008067 ret --------------------------------------------------------- After careful comparison, it was found that there are fewer assembly instructions after the patch. Thanks Xiao Zeng ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-16 9:30 ` Xiao Zeng @ 2023-12-20 4:24 ` Jeff Johnston 2023-12-20 5:51 ` Xiao Zeng 2023-12-20 22:24 ` Brian Inglis 0 siblings, 2 replies; 12+ messages in thread From: Jeff Johnston @ 2023-12-20 4:24 UTC (permalink / raw) To: Xiao Zeng; +Cc: Torbjorn SVENSSON, newlib, vapier, palmer, jeffreyalaw [-- Attachment #1: Type: text/plain, Size: 5632 bytes --] Patch merged to master. -- Jeff J. On Sat, Dec 16, 2023 at 4:31 AM Xiao Zeng <zengxiao@eswincomputing.com> wrote: > 2023-12-15 18:28 Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> wrote: > > > > >Hello Xiao, > > > >On 2023-12-15 09:31, Xiao Zeng wrote: > >> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com> > >> --- > >> newlib/libc/string/strcspn.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/newlib/libc/string/strcspn.c b/newlib/libc/string/strcspn.c > >> index abaa93ad6..8ac0bf10c 100644 > >> --- a/newlib/libc/string/strcspn.c > >> +++ b/newlib/libc/string/strcspn.c > >> @@ -37,12 +37,10 @@ strcspn (const char *s1, > >> for (c = s2; *c; c++) > >> { > >> if (*s1 == *c) > >> - break; > >> + goto end; > >> } > >> - if (*c) > >> - break; > >> s1++; > >> } > >> - > >> +end: > >> return s1 - s; > >> } > > > >Just looking at this small snippet of code, I would say that the > >previous code and your suggestion won't do the same thing. > > > >Do you have unit tests that confirm that the behavior is identical with > >the current implementation and your suggested change? > > > >When I run your suggestion, I get return value 0, but with the current > >implementation it's 3 for this call: strspn("129th", "1234567890"). > > > >Kind regards, > >Torbjörn > > After applying this patch, provide a comparison of assembly code > under the risc-v architecture, with default compilation parameters > used in both of them: > > no-patch: > --------------------------------------------------------- > libc_a-strcspn.o: file format elf64-littleriscv > > > Disassembly of section .text: > > 0000000000000000 <strcspn>: > 0: 00054683 lbu a3,0(a0) > 4: 06068263 beqz a3,68 <.L9> > 8: 0005c803 lbu a6,0(a1) > c: 00050613 mv a2,a0 > > 0000000000000010 <.LVL1>: > 10: 02080463 beqz a6,38 <.L14> > > 0000000000000014 <.L6>: > 14: 00058793 mv a5,a1 > 18: 00080713 mv a4,a6 > 1c: 00c0006f j 28 <.L5> > > 0000000000000020 <.L4>: > 20: 0007c703 lbu a4,0(a5) > 24: 02070863 beqz a4,54 <.L17> > > 0000000000000028 <.L5>: > 28: 00178793 addi a5,a5,1 > > 000000000000002c <.LVL5>: > 2c: fee69ae3 bne a3,a4,20 <.L4> > > 0000000000000030 <.L7>: > 30: 40a60533 sub a0,a2,a0 > > 0000000000000034 <.LVL7>: > 34: 00008067 ret > > 0000000000000038 <.L14>: > 38: 00164683 lbu a3,1(a2) > 3c: 00160613 addi a2,a2,1 > 40: fe0688e3 beqz a3,30 <.L7> > 44: 00164683 lbu a3,1(a2) > 48: 00160613 addi a2,a2,1 > 4c: fe0696e3 bnez a3,38 <.L14> > 50: fe1ff06f j 30 <.L7> > > 0000000000000054 <.L17>: > 54: 00164683 lbu a3,1(a2) > 58: 00160613 addi a2,a2,1 > 5c: fa069ce3 bnez a3,14 <.L6> > 60: 40a60533 sub a0,a2,a0 > > 0000000000000064 <.LVL13>: > 64: 00008067 ret > > 0000000000000068 <.L9>: > 68: 00000513 li a0,0 > > 000000000000006c <.LVL15>: > 6c: 00008067 ret > --------------------------------------------------------- > > patch > --------------------------------------------------------- > libc_a-strcspn.o: file format elf64-littleriscv > > > Disassembly of section .text: > > 0000000000000000 <strcspn>: > 0: 00054683 lbu a3,0(a0) > 4: 04068a63 beqz a3,58 <.L2> > 8: 0005c803 lbu a6,0(a1) > c: 00050613 mv a2,a0 > > 0000000000000010 <.LVL1>: > 10: 02080c63 beqz a6,48 <.L14> > > 0000000000000014 <.L6>: > 14: 00058793 mv a5,a1 > 18: 00080713 mv a4,a6 > 1c: 00c0006f j 28 <.L5> > > 0000000000000020 <.L4>: > 20: 0007c703 lbu a4,0(a5) > 24: 00070a63 beqz a4,38 <.L16> > > 0000000000000028 <.L5>: > 28: 00178793 addi a5,a5,1 > > 000000000000002c <.LVL5>: > 2c: fee69ae3 bne a3,a4,20 <.L4> > > 0000000000000030 <.L7>: > 30: 40a60533 sub a0,a2,a0 > > 0000000000000034 <.LVL7>: > 34: 00008067 ret > > 0000000000000038 <.L16>: > 38: 00164683 lbu a3,1(a2) > 3c: 00160613 addi a2,a2,1 > 40: fc069ae3 bnez a3,14 <.L6> > 44: fedff06f j 30 <.L7> > > 0000000000000048 <.L14>: > 48: 00164683 lbu a3,1(a2) > 4c: 00160613 addi a2,a2,1 > 50: fe069ce3 bnez a3,48 <.L14> > 54: fddff06f j 30 <.L7> > > 0000000000000058 <.L2>: > 58: 00000513 li a0,0 > > 000000000000005c <.LVL12>: > 5c: 00008067 ret > --------------------------------------------------------- > After careful comparison, it was found that there are fewer assembly > instructions after the patch. > > Thanks > Xiao Zeng > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-20 4:24 ` Jeff Johnston @ 2023-12-20 5:51 ` Xiao Zeng 2023-12-20 22:24 ` Brian Inglis 1 sibling, 0 replies; 12+ messages in thread From: Xiao Zeng @ 2023-12-20 5:51 UTC (permalink / raw) To: Jeff Johnston; +Cc: Torbjorn SVENSSON, newlib, vapier, palmer, jeffreyalaw 2023-12-20 12:24 Jeff Johnston <jjohnstn@redhat.com> wrote: > >Patch merged to master. Thanks, Jeff. There will be some similar patches in the future, and I will submit them to the master immediately. > >-- Jeff J. > >On Sat, Dec 16, 2023 at 4:31 AM Xiao Zeng <zengxiao@eswincomputing.com> >wrote: > >> 2023-12-15 18:28 Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> wrote: >> > >> >> >Hello Xiao, >> > >> >On 2023-12-15 09:31, Xiao Zeng wrote: >> >> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com> >> >> --- >> >> newlib/libc/string/strcspn.c | 6 ++---- >> >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/newlib/libc/string/strcspn.c b/newlib/libc/string/strcspn.c >> >> index abaa93ad6..8ac0bf10c 100644 >> >> --- a/newlib/libc/string/strcspn.c >> >> +++ b/newlib/libc/string/strcspn.c >> >> @@ -37,12 +37,10 @@ strcspn (const char *s1, >> >> for (c = s2; *c; c++) >> >> { >> >> if (*s1 == *c) >> >> - break; >> >> + goto end; >> >> } >> >> - if (*c) >> >> - break; >> >> s1++; >> >> } >> >> - >> >> +end: >> >> return s1 - s; >> >> } >> > >> >Just looking at this small snippet of code, I would say that the >> >previous code and your suggestion won't do the same thing. >> > >> >Do you have unit tests that confirm that the behavior is identical with >> >the current implementation and your suggested change? >> > >> >When I run your suggestion, I get return value 0, but with the current >> >implementation it's 3 for this call: strspn("129th", "1234567890"). >> > >> >Kind regards, >> >Torbjörn >> >> After applying this patch, provide a comparison of assembly code >> under the risc-v architecture, with default compilation parameters >> used in both of them: >> >> no-patch: >> --------------------------------------------------------- >> libc_a-strcspn.o: file format elf64-littleriscv >> >> >> Disassembly of section .text: >> >> 0000000000000000 <strcspn>: >> 0: 00054683 lbu a3,0(a0) >> 4: 06068263 beqz a3,68 <.L9> >> 8: 0005c803 lbu a6,0(a1) >> c: 00050613 mv a2,a0 >> >> 0000000000000010 <.LVL1>: >> 10: 02080463 beqz a6,38 <.L14> >> >> 0000000000000014 <.L6>: >> 14: 00058793 mv a5,a1 >> 18: 00080713 mv a4,a6 >> 1c: 00c0006f j 28 <.L5> >> >> 0000000000000020 <.L4>: >> 20: 0007c703 lbu a4,0(a5) >> 24: 02070863 beqz a4,54 <.L17> >> >> 0000000000000028 <.L5>: >> 28: 00178793 addi a5,a5,1 >> >> 000000000000002c <.LVL5>: >> 2c: fee69ae3 bne a3,a4,20 <.L4> >> >> 0000000000000030 <.L7>: >> 30: 40a60533 sub a0,a2,a0 >> >> 0000000000000034 <.LVL7>: >> 34: 00008067 ret >> >> 0000000000000038 <.L14>: >> 38: 00164683 lbu a3,1(a2) >> 3c: 00160613 addi a2,a2,1 >> 40: fe0688e3 beqz a3,30 <.L7> >> 44: 00164683 lbu a3,1(a2) >> 48: 00160613 addi a2,a2,1 >> 4c: fe0696e3 bnez a3,38 <.L14> >> 50: fe1ff06f j 30 <.L7> >> >> 0000000000000054 <.L17>: >> 54: 00164683 lbu a3,1(a2) >> 58: 00160613 addi a2,a2,1 >> 5c: fa069ce3 bnez a3,14 <.L6> >> 60: 40a60533 sub a0,a2,a0 >> >> 0000000000000064 <.LVL13>: >> 64: 00008067 ret >> >> 0000000000000068 <.L9>: >> 68: 00000513 li a0,0 >> >> 000000000000006c <.LVL15>: >> 6c: 00008067 ret >> --------------------------------------------------------- >> >> patch >> --------------------------------------------------------- >> libc_a-strcspn.o: file format elf64-littleriscv >> >> >> Disassembly of section .text: >> >> 0000000000000000 <strcspn>: >> 0: 00054683 lbu a3,0(a0) >> 4: 04068a63 beqz a3,58 <.L2> >> 8: 0005c803 lbu a6,0(a1) >> c: 00050613 mv a2,a0 >> >> 0000000000000010 <.LVL1>: >> 10: 02080c63 beqz a6,48 <.L14> >> >> 0000000000000014 <.L6>: >> 14: 00058793 mv a5,a1 >> 18: 00080713 mv a4,a6 >> 1c: 00c0006f j 28 <.L5> >> >> 0000000000000020 <.L4>: >> 20: 0007c703 lbu a4,0(a5) >> 24: 00070a63 beqz a4,38 <.L16> >> >> 0000000000000028 <.L5>: >> 28: 00178793 addi a5,a5,1 >> >> 000000000000002c <.LVL5>: >> 2c: fee69ae3 bne a3,a4,20 <.L4> >> >> 0000000000000030 <.L7>: >> 30: 40a60533 sub a0,a2,a0 >> >> 0000000000000034 <.LVL7>: >> 34: 00008067 ret >> >> 0000000000000038 <.L16>: >> 38: 00164683 lbu a3,1(a2) >> 3c: 00160613 addi a2,a2,1 >> 40: fc069ae3 bnez a3,14 <.L6> >> 44: fedff06f j 30 <.L7> >> >> 0000000000000048 <.L14>: >> 48: 00164683 lbu a3,1(a2) >> 4c: 00160613 addi a2,a2,1 >> 50: fe069ce3 bnez a3,48 <.L14> >> 54: fddff06f j 30 <.L7> >> >> 0000000000000058 <.L2>: >> 58: 00000513 li a0,0 >> >> 000000000000005c <.LVL12>: >> 5c: 00008067 ret >> --------------------------------------------------------- >> After careful comparison, it was found that there are fewer assembly >> instructions after the patch. >> >> Thanks >> Xiao Zeng >> >> Thanks Xiao Zeng ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-20 4:24 ` Jeff Johnston 2023-12-20 5:51 ` Xiao Zeng @ 2023-12-20 22:24 ` Brian Inglis 2023-12-21 8:00 ` Stefan Tauner 1 sibling, 1 reply; 12+ messages in thread From: Brian Inglis @ 2023-12-20 22:24 UTC (permalink / raw) To: newlib On 2023-12-19 21:24, Jeff Johnston wrote: > Patch merged to master. > On Sat, Dec 16, 2023 at 4:31 AM Xiao Zeng wrote: > 2023-12-15 18:28 Torbjorn SVENSSON wrote: >> On 2023-12-15 09:31, Xiao Zeng wrote: >>> Signed-off-by: Xiao Zeng <zengxiao@eswincomputing.com > <mailto:zengxiao@eswincomputing.com>> >>> --- >>> newlib/libc/string/strcspn.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/newlib/libc/string/strcspn.c b/newlib/libc/string/strcspn.c >>> index abaa93ad6..8ac0bf10c 100644 >>> --- a/newlib/libc/string/strcspn.c >>> +++ b/newlib/libc/string/strcspn.c >>> @@ -37,12 +37,10 @@ strcspn (const char *s1, > >> for (c = s2; *c; c++) > >> { > >> if (*s1 == *c) > >> - break; > >> + goto end; > >> } > >> - if (*c) > >> - break; > >> s1++; > >> } > >> - > >> +end: > >> return s1 - s; > >> } >> Just looking at this small snippet of code, I would say that the previous >> code and your suggestion won't do the same thing. >> >> Do you have unit tests that confirm that the behavior is identical with the >> current implementation and your suggested change? >> >> When I run your suggestion, I get return value 0, but with the current >> implementation it's 3 for this call: strspn("129th", "1234567890"). > After applying this patch, provide a comparison of assembly code under the > risc-v architecture, with default compilation parameters used in both of > them: These "micro-optimizations" improve code generation by a few instructions on a single (RISC-V) target at a single optimization level of a single compiler and version, but what is the cost in execution time and the cache imoact? Using gotos throw away potential optimizations in modern compilers, where goto-less code may have control and/or data flow optimized, with branches altered or eliminated, depending on target instruction sets and compiler supported optimizations selected and implemented. For example, in these small functions with few branches, conditional execution instructions could be generated, eliminating branches, cache, and lookaside buffer impacts, possibly allowing inlining. Who knows what impacts this has on all of the other targets, compilers, versions, and optimization levels? Should we even consider making these kinds of non-bug-fix minor changes to non-target specific sources, unless there are algorithm changes with demonstrated benefits across multiple targets, compilers, versions, and optimization levels? -- Take care. Thanks, Brian Inglis Calgary, Alberta, Canada La perfection est atteinte Perfection is achieved non pas lorsqu'il n'y a plus rien à ajouter not when there is no more to add mais lorsqu'il n'y a plus rien à retirer but when there is no more to cut -- Antoine de Saint-Exupéry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-20 22:24 ` Brian Inglis @ 2023-12-21 8:00 ` Stefan Tauner 2023-12-21 17:47 ` Brian Inglis 0 siblings, 1 reply; 12+ messages in thread From: Stefan Tauner @ 2023-12-21 8:00 UTC (permalink / raw) To: newlib On Wed, 20 Dec 2023 15:24:10 -0700 Brian Inglis <Brian.Inglis@SystematicSW.ab.ca> wrote: > Using gotos throw away potential optimizations in modern compilers {{citation needed}} -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization 2023-12-21 8:00 ` Stefan Tauner @ 2023-12-21 17:47 ` Brian Inglis 0 siblings, 0 replies; 12+ messages in thread From: Brian Inglis @ 2023-12-21 17:47 UTC (permalink / raw) To: newlib On 2023-12-21 01:00, Stefan Tauner wrote: > On Wed, 20 Dec 2023 15:24:10 -0700 Brian Inglis wrote: >> Using gotos throw away potential optimizations in modern compilers > {{citation needed}} Please note that is a simplification to make the point. No gotos means reducible flow graphs which allow easier analysis and optimization. The main problem is the creation of irreducible flow graphs using gotos. Compilers may be able to convert irreducible flow graphs to reducible flow graphs, eliminating the gotos by restructuring. This may then allow the code to meet preconditions allowing further loop or block optimizations designed for structured reducible flow graphs. See: https://groups.seas.harvard.edu/courses/cs153/2019fa/lectures/Lec23-Loop-optimization.pdf https://escholarship.mcgill.ca/concern/theses/x633f2516 -- Take care. Thanks, Brian Inglis Calgary, Alberta, Canada La perfection est atteinte Perfection is achieved non pas lorsqu'il n'y a plus rien à ajouter not when there is no more to add mais lorsqu'il n'y a plus rien à retirer but when there is no more to cut -- Antoine de Saint-Exupéry [I was stunned into more structured approaches early on by having to work on a large monolithic uncommented Fortran program that used *only* assigned gotos with meaningless random numeric labels and dozens of human named label variables e.g. ... 813 ASSIGN 105 to FRED GOTO JIM 534 IF (I.GT.0) GO TO JOHN GOTO JACK 105 ASSIGN 534 to ANDY GOTO MIKE ... interspersed with some matrix calculations. Aaaargh!] ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-12-21 17:47 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-15 8:31 [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization Xiao Zeng 2023-12-15 10:28 ` Torbjorn SVENSSON 2023-12-16 4:45 ` Xiao Zeng 2023-12-16 4:56 ` Xiao Zeng 2023-12-20 15:08 ` Torbjorn SVENSSON 2023-12-21 1:22 ` Xiao Zeng 2023-12-16 9:30 ` Xiao Zeng 2023-12-20 4:24 ` Jeff Johnston 2023-12-20 5:51 ` Xiao Zeng 2023-12-20 22:24 ` Brian Inglis 2023-12-21 8:00 ` Stefan Tauner 2023-12-21 17:47 ` Brian Inglis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).