This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Optimize ranges::count for __bit_iterators
ClosedPublic

Authored by philnik on Aug 2 2023, 5:31 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGa9138cdb36cc: [libc++] Optimize ranges::count for __bit_iterators
Summary
---------------------------------------------------------------
Benchmark                                    old            new
---------------------------------------------------------------
bm_vector_bool_count/1                   1.92 ns        1.92 ns
bm_vector_bool_count/2                   1.92 ns        1.92 ns
bm_vector_bool_count/3                   1.92 ns        1.92 ns
bm_vector_bool_count/4                   1.92 ns        1.92 ns
bm_vector_bool_count/5                   1.92 ns        1.92 ns
bm_vector_bool_count/6                   1.92 ns        1.92 ns
bm_vector_bool_count/7                   1.92 ns        1.92 ns
bm_vector_bool_count/8                   1.92 ns        1.92 ns
bm_vector_bool_count/16                  1.92 ns        1.92 ns
bm_vector_bool_count/64                  2.24 ns        2.25 ns
bm_vector_bool_count/512                 3.19 ns        3.20 ns
bm_vector_bool_count/4096                14.1 ns        12.3 ns
bm_vector_bool_count/32768               84.0 ns        83.6 ns
bm_vector_bool_count/262144               664 ns         661 ns
bm_vector_bool_count/1048576             2623 ns        2628 ns
bm_vector_bool_ranges_count/1            1.07 ns        1.92 ns
bm_vector_bool_ranges_count/2            1.65 ns        1.92 ns
bm_vector_bool_ranges_count/3            2.27 ns        1.92 ns
bm_vector_bool_ranges_count/4            2.68 ns        1.92 ns
bm_vector_bool_ranges_count/5            3.33 ns        1.92 ns
bm_vector_bool_ranges_count/6            3.99 ns        1.92 ns
bm_vector_bool_ranges_count/7            4.67 ns        1.92 ns
bm_vector_bool_ranges_count/8            5.19 ns        1.92 ns
bm_vector_bool_ranges_count/16           11.1 ns        1.92 ns
bm_vector_bool_ranges_count/64           52.2 ns        2.24 ns
bm_vector_bool_ranges_count/512           452 ns        3.20 ns
bm_vector_bool_ranges_count/4096         3577 ns        12.1 ns
bm_vector_bool_ranges_count/32768       28725 ns        83.7 ns
bm_vector_bool_ranges_count/262144     229676 ns         662 ns
bm_vector_bool_ranges_count/1048576    905574 ns        2625 ns

Diff Detail

Event Timeline

philnik created this revision.Aug 2 2023, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2023, 5:31 PM
philnik requested review of this revision.Aug 2 2023, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2023, 5:31 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik edited the summary of this revision. (Show Details)Aug 2 2023, 5:39 PM
philnik updated this revision to Diff 546898.Aug 3 2023, 8:59 AM

Try to fix CI

philnik updated this revision to Diff 547055.Aug 3 2023, 5:02 PM

Try to fix CI

ldionne requested changes to this revision.Aug 18 2023, 10:19 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/__algorithm/count.h
35

We normally name these algorithms just __count.

76

Can you make sure that these code paths are tested? We could have a test like libcxx/test/std/algorithms/alg.nonmodifying/alg.count/count.vector_bool.pass.cpp *or* libcxx/test/std/containers/sequences/vector.bool/count.pass.cpp, but we should have something.

I would recommend breaking this logic in a very obvious way to first see whether we have an existing test for this.

libcxx/include/__algorithm/iterator_operations.h
176–177 ↗(On Diff #547055)

I think I would avoid introducing this, I don't think it provides that much value.

This revision now requires changes to proceed.Aug 18 2023, 10:19 AM
philnik updated this revision to Diff 557231.Sep 22 2023, 2:02 AM
philnik marked 2 inline comments as done.

Address comments

philnik updated this revision to Diff 557266.Sep 23 2023, 12:57 AM

Try to fix CI

ldionne accepted this revision.Sep 28 2023, 9:31 AM

LGTM with comments applied and CI passing.

libcxx/include/__algorithm/count.h
46

And we should make sure that we have a test for that, i.e. call std::count(bit-iterator) and ranges::count(bit-iterator) in constexpr in C++20 mode.

libcxx/include/__algorithm/iterator_operations.h
176–177 ↗(On Diff #547055)

^

I wouldn't necessarily object to introducing these types, but we should do that separately and handle not only __difference_type.

This revision is now accepted and ready to land.Sep 28 2023, 9:31 AM
ldionne added inline comments.Sep 28 2023, 9:31 AM
libcxx/test/std/algorithms/alg.nonmodifying/alg.count/count.pass.cpp
1

We need a test with ranges::count and std::vector<bool> too.

philnik updated this revision to Diff 557517.Oct 1 2023, 5:53 AM
philnik marked 4 inline comments as done.

Try to fix CI

philnik updated this revision to Diff 557626.Oct 6 2023, 2:39 AM

Fix ignore_format.txt

This revision was landed with ongoing or failed builds.Oct 6 2023, 1:58 PM
This revision was automatically updated to reflect the committed changes.