Page MenuHomePhabricator

[libc++] Optimize algorithms on uninitialized memory for trivial types.
Needs ReviewPublic

Authored by var-const on Jan 27 2022, 12:43 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

There are five cases (also applying to the *_n versions of the
algorithms):

  • destroy is a no-op when the type is trivially destructible;
  • uninitialized_default_construct is a no-op when the type is trivially default-constructible and destructible;
  • uninitialized_value_construct zeroes out trivially default constructible types using memset;
  • uninitialized_fill uses memset if both types are trivial and 1-byte long;
  • uninitialized_copy and uninitialized_move use memcpy on trivially copy-constructible types.

Diff Detail

Event Timeline

var-const requested review of this revision.Jan 27 2022, 12:43 AM
var-const created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 12:43 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Fix tests in C++11 - C++17 modes.

Make construct_at.h compile in C++03 mode.

FWIW, libc++'s historical position on this kind of optimization (which I associate particularly with Marshall's tenure) is "libc++ should do the simple thing; file a codegen bug against Clang if it's failing to optimize our simple template code for some trivial type." TBF, I have not looked closely at this PR to know whether that attitude would be unfair here for some reason.
Do you have a Godbolt of the codegen before and after? (You should.)

FWIW, libc++'s historical position on this kind of optimization (which I associate particularly with Marshall's tenure) is "libc++ should do the simple thing; file a codegen bug against Clang if it's failing to optimize our simple template code for some trivial type." TBF, I have not looked closely at this PR to know whether that attitude would be unfair here for some reason.
Do you have a Godbolt of the codegen before and after? (You should.)

FWIW, I "sponsored" this patch in that I believe I was the one to request that we do it (or if not, then I immediately thought it was a good idea). I've also gone through several review iterations on it with Costa before it got published, so this is not far from a LGTM from my side.

However, I do agree it should be backed by at least some proof that it improves codegen. If Clang always ends up doing the right thing without us having to lift a finger, then this complexity is not actually worth it since the naive code "just works". I'd be surprised if Clang was clever enough to perform these optimizations in all cases, but I could be surprised.

FWIW, libc++'s historical position on this kind of optimization (which I associate particularly with Marshall's tenure) is "libc++ should do the simple thing; file a codegen bug against Clang if it's failing to optimize our simple template code for some trivial type." TBF, I have not looked closely at this PR to know whether that attitude would be unfair here for some reason.
Do you have a Godbolt of the codegen before and after? (You should.)

@Quuxplusone This is a very good point. I'd suggest holding off the review for now until I have some data.

FWIW, libc++'s historical position on this kind of optimization (which I associate particularly with Marshall's tenure) is "libc++ should do the simple thing; file a codegen bug against Clang if it's failing to optimize our simple template code for some trivial type." TBF, I have not looked closely at this PR to know whether that attitude would be unfair here for some reason.
Do you have a Godbolt of the codegen before and after? (You should.)

@Quuxplusone This is a very good point. I'd suggest holding off the review for now until I have some data.

Thanks, I too like to see this data.

I only skimmed over the patch and didn't do a review.

libcxx/include/__memory/construct_at.h
113

For an if constexpr there needs to be an else branch to guarantee the code below isn't used.

Please search the patch for other occurances of if constexpr.

huixie90 added inline comments.
libcxx/include/__memory/construct_at.h
80

Why is value_type used?
_VSTD::__destroy_at(_VSTD::addressof(*__first)); does not have any value_type involved

libcxx/include/__memory/uninitialized_algorithms.h
299

why is_trivially_destructible_v is needed? IIUC, we are not destroying anything since the memory [first, last) does not have any objects yet thus no destructors are required

300–308

This looks like the implementation of ranges::next(__first, __last) but I guess it is hard to reuse because this algorithm is c++17?

Herald added a project: Restricted Project. · View Herald TranscriptMon, Aug 8, 11:00 AM