This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add memset and bzero implementations
ClosedPublic

Authored by gchatelet on May 15 2020, 7:32 AM.

Details

Summary

This patch adds general purpose memset and bzero implementations.

Diff Detail

Event Timeline

gchatelet created this revision.May 15 2020, 7:32 AM
gchatelet marked an inline comment as done.May 15 2020, 7:36 AM
gchatelet added inline comments.
libc/src/string/memory_utils/memset_utils.h
20

Theoretically the compiler is allowed to call memsethere and end up with a recursive call, practically it doesn't happen, however this should be replaced with a __builtin_memset_inline once it's available in clang.

gchatelet updated this revision to Diff 264242.May 15 2020, 7:40 AM
  • Remove dead code.
abrachet added inline comments.May 15 2020, 11:44 AM
libc/src/string/memory_utils/memset_utils.h
10

Should this have STRING in between SRC and MEMORY? I think clang-tidy might complain.

20

Of course I can't find it anymore, but we had for memcpy a CMake rule to see if the file contained any references to memcpy I think. Is that being done here?

28

Strange const

96

Why choose 32 and not 64? If 32 is the best choice here then why does the count <= 128 condition exist?

libc/src/string/memset.cpp
17–18

I think the cast is explanatory enough without the comment.

libc/test/src/string/bzero_test.cpp
1

//===-- Unittests for bzero --...

41

There was recently a patch which changed all uses of functional style casts. I don't know if its in the style guide to not use them or not. But maybe it's better to use (char) 0 or '\0'?

libc/test/src/string/memcpy_test.cpp
39

Maybe reinterpret_cast to const void *const. Obviously it isn't harmful here because ASSERT_EQ takes a const T but there is a sneaky const_cast in this C-style cast

gchatelet marked 11 inline comments as done.May 18 2020, 6:45 AM
gchatelet added inline comments.
libc/src/string/memory_utils/memset_utils.h
20

Yes I removed them because it was brittle, also we now have tests that cover all implementations so the stack overflow will be caught.

96

This may look surprising indeed. We want to balance two things here:

  • The number of redundant writes,
  • The number of conditionals for sizes <=128 : 91% of memset calls are for such sizes.

For the range 64-128:

  • SetBlockOverlap<64> uses no conditionals but always writes 128 Bytes this is wasteful for 65 but efficient for 128.
  • SetAlignedBlocks<32> would consume between 3 and 4 conditionals and write 96 or 128 Bytes.
  • Another approach could be to use an hybrid approach Copy<64>+Overlap<32> for 65-96 and Copy<96>+Overlap<32> for 97-128

Benchmarks showed that redundant writes were cheap (for Intel X86) but conditional were expensive, even on processor that do not support writing 64B at a time (pre-AVX512F). We also want to favor short functions that allow more hot code to fit in the iL1 cache.

Above 128 we have to use conditionals since we don't know the upper bound in advance. SetAlignedBlocks<64> may waste up to 63 Bytes, SetAlignedBlocks<32> may waste up to 31 Bytes. Benchmarks showed that SetAlignedBlocks<64> was not superior for sizes that mattered.

To be honest this part of the implementation is subject to change as we benchmark more processors. We may also want to specialize it for processors with specialized instructions that performs better (rep stosb).

libc/test/src/string/bzero_test.cpp
41

This is not a functional cast but a constructor of type char with value 0.

gchatelet updated this revision to Diff 264608.May 18 2020, 6:45 AM
gchatelet marked 3 inline comments as done.
  • Adress comments
sivachandra accepted this revision.May 18 2020, 9:20 AM

LGTM overall.

May be a tad incomplete in the sense that bzero will not show up in the public string.h. But, I don't think this patch should block on that. bzero is essentially an LLVM libc extension at this point. It probably makes sense to add a new standard spec called llvm-libc-ext and add all llvm-libc specific functions there and then expose them via public headers.

libc/src/string/memory_utils/memset_utils.h
20

Can you add your note as a comment here?

96

Can you add your explanation as a comment here?

This revision is now accepted and ready to land.May 18 2020, 9:20 AM
gchatelet updated this revision to Diff 264905.May 19 2020, 7:36 AM
  • Address comments

I'll submit this version and we can iterate from here. Thx for the review !

This revision was automatically updated to reflect the committed changes.