public inbox for newlib@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization
@ 2023-12-22  7:53 Xiao Zeng
  0 siblings, 0 replies; 7+ messages in thread
From: Xiao Zeng @ 2023-12-22  7:53 UTC (permalink / raw)
  To: newlib; +Cc: Brian.Inglis, jjohnstn, stefan.tauner

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


^ permalink raw reply	[flat|nested] 7+ messages in thread
* [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; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2023-12-22  7:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-22  7:53 [PATCH] newlib: libc: Improved the readability of strcspn with minor optimization Xiao Zeng
  -- strict thread matches above, loose matches on Subject: below --
2023-12-15  8:31 Xiao Zeng
2023-12-15 10:28 ` Torbjorn SVENSSON
2023-12-16  4:45   ` Xiao Zeng
2023-12-20 15:08     ` Torbjorn SVENSSON
2023-12-16  9:30   ` Xiao Zeng
2023-12-20  4:24     ` Jeff Johnston
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).