Details

Reviewers

jhibbits
nwhitehorn
breno.leitao_gmail.com
leonardo.bianconi_eldorado.org.br

Commits

rS356766: [PPC64] strncpy optimization

Summary

Update: strncpy.S optimized

Since PowerPC has hardware support for memory alignment, the initial session was removed. Also, the copy-by-byte session (Lstd_byte) was replaced by a faster code.

Performance gain comparing to the former strncpy.S

String size (bytes)	size <= 8	16 <= size <= 32	64 <= size <= 128	256 <= size <= 512	1024 <= size <= 2048
Gain rate	0.17%	0.31%	0.62%	0.37%	0.22%

Performance gain comparing to the original strncpy (linked in libc):

String size (bytes)	size <= 8	16 <= size <= 32	64 <= size <= 128	256 <= size <= 512	1024 <= size <= 2048
Gain rate	0.23%	2.48%	8.30%	25.81%	55.12%

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25625
Build 24219: arc lint + arc unit

Event Timeline

leonardo.bianconi_eldorado.org.br created this revision.May 9 2018, 3:51 PM

Herald added a subscriber: lffpires_ruabrasil.org. · View Herald TranscriptMay 9 2018, 3:51 PM

Harbormaster completed remote builds in B16497: Diff 42319.May 9 2018, 3:51 PM

leonardo.bianconi_eldorado.org.br edited the summary of this revision. (Show Details)May 9 2018, 4:00 PM

leonardo.bianconi_eldorado.org.br edited the summary of this revision. (Show Details)May 9 2018, 4:04 PM

leonardo.bianconi_eldorado.org.br added reviewers: jhibbits, nwhitehorn, breno.leitao_gmail.com.May 9 2018, 4:09 PM

leonardo.bianconi_eldorado.org.br added a project: PowerPC.

leonardo.bianconi_eldorado.org.br added subscribers: alexandre.yamashita_eldorado.org.br, leandro.lupori_gmail.com, breno.leitao_gmail.com.

alexandre.osorio_eldorado.org.br added a subscriber: alexandre.osorio_eldorado.org.br.Mar 19 2019, 7:44 PM

Optimizing strncpy.S
Since PowerPC has hardware support for memory alignment, the initial session was removed. Also, the copy-by-byte session (Lstd_byte) was replaced by a faster code.

Related to the former code, the performance gain rates are:

bytesToCopy <= 8 : 0.17%
16 <= bytesToCopy <= 32 : 0.50%
64 <= bytesToCopy <= 128 : 0.60%
256 <= bytesToCopy <= 512 : 0.33%
1024 <= bytesToCopy <= 2048 : 0.19%

Harbormaster completed remote builds in B23216: Diff 55289.Mar 20 2019, 4:37 PM

alexandre.osorio_eldorado.org.br edited the summary of this revision. (Show Details)Mar 20 2019, 5:25 PM

alexandre.osorio_eldorado.org.br edited the summary of this revision. (Show Details)

Please, Can anyone review this?

leonardo.bianconi_eldorado.org.br retitled this revision from strncpy optimization fir PowerPC64 to strncpy optimization for PowerPC64.Apr 12 2019, 12:36 PM

Can you test on misaligned buffers?

Also, this appears to be the only diff that uses raw numbers for registers, instead of using the '%r*' convention. Can you adjust to match the rest of the style?

This revision now requires changes to proceed.Apr 17 2019, 5:02 PM

luporl commandeered this revision.Jun 5 2019, 6:18 PM

luporl added a reviewer: leonardo.bianconi_eldorado.org.br.

I think this, and the other related string optimizations, needs revisited regarding alignment. I tested this on a e5500 based system, and glib segfaulted constantly, due to string issues. The problem is that misaligned accesses can cross a page boundary on the last byte(s). If the following page is unmapped, a fault occurs, even if the copy is valid.

Use %r* instead of raw numbers for registers

Harbormaster completed remote builds in B25248: Diff 59538.Jul 8 2019, 4:40 PM

Changed register convention.
Still need to investigate and fix the misaligned access issue.

Handled misalignment issues
Improved implementation
Now using memset() for faster zeroing of dst buffer

Harbormaster completed remote builds in B25349: Diff 59760.Jul 15 2019, 1:19 PM

I consider the strncpy implementation complete now.
Adding ifunc to select the optimized implementation, however, is still not possible, as rtld doesn't support it yet.

Have you checked performance on this latest version?

In D15369#456889, @jhibbits wrote:

Have you checked performance on this latest version?

Yes, I have. It is practically the same as that of the last version, except for cases where 'n' is bigger than the string length. In this case there is a big improvement, as the "zero the rest of the buffer" part was being done byte by byte and now memset() is used instead.

jhibbits added inline comments.Jul 24 2019, 3:39 PM

lib/libc/powerpc64/string/strncpy.S
101 ↗	(On Diff #59760)	Wouldn't we want to zero the remaining if 0 is found earlier?

luporl added inline comments.Jul 24 2019, 3:56 PM

lib/libc/powerpc64/string/strncpy.S
101 ↗	(On Diff #59760)	Yes, you're right, I've missed this case. I'll fix it.

[PPC64] strncpy - fix 'dst' not zeroed issue
Initial ifunc capable strncpy implementation

Harbormaster completed remote builds in B25619: Diff 60328.Jul 31 2019, 6:37 PM

luporl marked an inline comment as done.Jul 31 2019, 6:41 PM

This last change fixes the previous "dst not always zeroed" issue and adds ifunc support to strncpy, so that the optimized version is selected on ISAs >= 2.05 while others can use the fallback implementation in C.

While it works, I've raise a few questions/issues/points to improve in the following comments.

lib/libc/powerpc64/string/strncpy.c
35–36	There should be a better way to reuse the C implementation of strncpy... Suggestions?
38–39	This corresponds to the optimized assembly implementation, that is in another .o file. I've confirmed that this works, but I think I've read somewhere that ifunc resolvers should choose between functions from the same compilation unit. Does anyone know about such limitation?
lib/libc/string/strncpy.c
52	Maybe strncpy() should also be static when included? Or use a reserved identifier (__strncpy_c)?

luporl added a parent revision: D21070: [PPC64] Add ifunc support in libcsu.Jul 31 2019, 7:01 PM

[PPC64] strncpy - fix rtld-libc build issue

Herald added a subscriber: bdragon. · View Herald TranscriptJul 31 2019, 9:31 PM

Harbormaster completed remote builds in B25625: Diff 60343.Jul 31 2019, 9:31 PM

[PPC64] strncpy - fix rtld crash

Avoid using ifuncs in rtld.

Although it could be possible to make rtld capable of resolving
its own ifunc relocations, this would add complexity and make
the parts of code executed before ifuncs were relocated dangerous,
as any call to an unresolved ifunc would most probably result in
a crash.

Harbormaster completed remote builds in B25644: Diff 60393.Aug 2 2019, 4:10 PM

luporl added inline comments.Aug 2 2019, 4:18 PM

lib/libc/powerpc64/string/Makefile.inc
6	This contains the common, C implementation of strncpy(). It is exported as a weak symbol, to make it possible for binaries like rtld to use the non-ifunc version of the function. It is also made available as __strncpy(), allowing the resolver to reference it.

Merge branch 'master'

Harbormaster completed remote builds in B27042: Diff 63289.Oct 15 2019, 2:21 PM

Fix diff

alfredo added a subscriber: alfredo.Nov 22 2019, 4:38 PM

luporl added a subscriber: PowerPC.Dec 4 2019, 11:50 AM

luporl removed a subscriber: PowerPC.Dec 4 2019, 11:58 AM

jhibbits accepted this revision.Dec 12 2019, 5:07 PM

This revision is now accepted and ready to land.Dec 12 2019, 5:07 PM

jhibbits added inline comments.Dec 12 2019, 5:08 PM

lib/libc/powerpc64/string/strncpy_resolver.c
44 ↗	(On Diff #63290)	Wait, where does hwcap come from? It's static in lib/libc/gen/auxv.c.

This depends on the ifunc support code landing first, of course.

lib/libc/powerpc64/string/strncpy_resolver.c
44 ↗	(On Diff #63290)	Agree. In my current code for the ifunc prototype (which I need to submit real soon now), I have r3 called "cpu_features" and r4 called "cpu_features2", NOT hwcap/hwcap2.

luporl added inline comments.Dec 12 2019, 6:11 PM

lib/libc/powerpc64/string/strncpy_resolver.c
44 ↗	(On Diff #63290)	Hmm, hwcap comes from my version of the ifunc.h file, from DEFINE_UIFUNC macro. So, in version of ifunc.h that will be commited it will be called cpu_features instead?

bdragon added inline comments.Dec 12 2019, 6:39 PM

lib/libc/powerpc64/string/strncpy_resolver.c
44 ↗	(On Diff #63290)	Yeah, I went through a few iterations on it. Here's a snapshot of what's in my "superglue" integration branch at the moment: https://raw.githubusercontent.com/freebsd/freebsd/c37b3144d0537f0bac9d1ed445c4a2a296954425/sys/powerpc/include/ifunc.h I'll try and get around to finally getting "RTLD WIP3" fully cleaned up and ready today, that's been on my todo list for too long.

luporl added inline comments.Dec 12 2019, 7:04 PM

lib/libc/powerpc64/string/strncpy_resolver.c
44 ↗	(On Diff #63290)	Ok, I'll change this part. The other libc optimizations may need this change too, I'll need to check. Nice, ifunc support in RTLD is the last missing piece blocking libc optimizations and ifunc use in general. Well, that and the ELFv2 switch :)