Page MenuHomePhabricator

[libcxx] Improve code generation for vector::clear().
ClosedPublic

Authored by brucem on Oct 4 2016, 9:12 AM.

Details

Summary

By manipulating a local variable in the loop, when the loop can
be optimized away (due to no non-trivial destructors), this lets
it be fully optimized away and we modify the __end_ separately.

This results in a substantial improvement in the generated code.

Prior to this change, this would be generated (on x86_64):

movq    (%rdi), %rdx
movq    8(%rdi), %rcx
cmpq    %rdx, %rcx
je    LBB2_2
leaq    -12(%rcx), %rax
subq    %rdx, %rax
movabsq    $-6148914691236517205, %rdx ## imm = 0xAAAAAAAAAAAAAAAB
mulq    %rdx
shrq    $3, %rdx
notq    %rdx
leaq    (%rdx,%rdx,2), %rax
leaq    (%rcx,%rax,4), %rax
movq    %rax, 8(%rdi)

And after:

movq    (%rdi), %rax
movq    %rax, 8(%rdi)

This brings this in line with what other implementations do.

Diff Detail

Repository
rL LLVM

Event Timeline

brucem updated this revision to Diff 73493.Oct 4 2016, 9:12 AM
brucem retitled this revision from to [libcxx] Improve code generation for vector::clear()..
brucem updated this object.
brucem added a subscriber: cfe-commits.
brucem set the repository for this revision to rL LLVM.
mclow.lists edited edge metadata.Oct 4 2016, 10:56 AM

I had no idea that we had no tests for vector::clear. Oosps.

This looks good to me, but I want to play with the codegen for a bit before approving it.
Also, you should add cfe-commits to the subscribers.

thanks for doing this!

test/std/containers/sequences/vector/vector.modifiers/clear.pass.cpp
25 ↗(On Diff #73493)

How about a couple more assertions here:

LIBCPP_ASSERT(c.__invariants());`
LIBCPP_ASSERT(is_contiguous_container_asan_correct(c));
32 ↗(On Diff #73493)

Same here.

brucem updated this revision to Diff 73790.Oct 6 2016, 6:30 AM
brucem edited edge metadata.

Address comments from mclow.

brucem marked 2 inline comments as done.Oct 20 2016, 3:06 AM

These have been addressed, so this should be good for further review.

mclow.lists accepted this revision.Oct 25 2016, 6:14 PM
mclow.lists edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 25 2016, 6:14 PM

This was accepted long ago, and then I got sidetracked for a while. Is this still okay to land?

EricWF accepted this revision.Mar 23 2017, 6:59 AM
EricWF added a subscriber: EricWF.

Still LGTM

test/std/containers/sequences/vector/vector.modifiers/clear.pass.cpp
18 ↗(On Diff #73790)

LIBCPP_ASSERT requires including test_macros.h

This revision was automatically updated to reflect the committed changes.