[PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[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

Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Torbjorn SVENSSON]

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

Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Xiao Zeng]

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

Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Xiao Zeng]

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

Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Xiao Zeng]

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

Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Jeff Johnston]

[-- 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
>
>

Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Xiao Zeng]

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

Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Torbjorn SVENSSON]

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

Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Brian Inglis]

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

Re: Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Xiao Zeng]

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

Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Stefan Tauner]

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

Re: [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Brian Inglis]

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!]

[PATCH] newlib: libc: Improved the readability of strcspn with minor optimization

[Xiao Zeng]

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?
I'm very sorry, I've been busy with other things lately and haven't replied to your email.
Please believe that your feedback has been very helpful to me.

Yes, you provided a perspective on code optimization.

> 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
I need to clarify my motivation for modifying this code:
When I first saw this code, the redundancy check confused me. After trying to
modify the code, it becomes easier to understand, at least for me.

I use the risc-v architecture to demonstrate assembly comparison because I
happen to have a set of such toolchains at hand, and I work under the risc-v
architecture, which makes it easier for me to assess the effectiveness of code optimization.

As a junior compiler engineer, I would like to provide some information:
For branch jumps, there is a Zicond extension set in the risc-v architecture that can
turn them into sequential execution. The advantage is that this usually benefits the
speed of code execution, while the disadvantage is that the generated sequential
instruction sequences are coupled together, making it impossible to execute concurrently.

In the case of instruction support (such as Zicond mentioned earlier), the compiler
can only merge and optimize basic blocks in simple situations

Overall:
If branches can be removed from the code, it will increase the possibility of
compiler optimization. If redundant branches are allowed to exist, the compiler may not
be able to recognize this optimization (in strpbrk.c, because it involves speculation about
register values, the compiler may not be able to recognize this optimization)

For the code in newlib, I believe that faster code is better without significantly reducing
its readability and portability. (Of course, it's just my opinion)

Although I have not tested other architectures, based on my current knowledge, other
architectures will also receive better code after my patch. Can someone help test the
performance under other architectures and compilation parameters? thank you.
 
Thanks
Xiao Zeng