Page MenuHomePhabricator

Introduce a generic operator to apply complex operations to BitVector

Authored by serge-sans-paille on Mar 8 2021, 6:11 AM.



This avoids temporary and memcpy call when computing large expressions.

It's basically some kind of poor man's expression template, but it seems easier
to maintain to have a single generic apply call instead of the whole
expression template machinery here.

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Mar 8 2021, 6:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2021, 6:11 AM

Are you seeing any changes in performance from this?

With the patch, running perf stat ./bin/clang -w -c sqlite3.c -o/dev/null (basically compiling the SQLite amalgametion), I get

5,959,419,554      instructions

without the patch, I get

5,970,690,708      instructions

Please can you add more test coverage in BitVectorTest.cpp ?

@zturner Any comments?

OK - please would it be possible to get this checked for compile time tracker regressions? Otherwise I have no strong objections to this.

nikic added inline comments.Mar 22 2021, 11:05 AM

Shouldn't this be max_element rather than min_element?

(Or, can we change this class to make operations on differently-sized vectors illegal? This is hard to reason about.)

RKSimon requested changes to this revision.Mar 22 2021, 11:53 AM

Please add BitVectorTest.cpp test coverage (inc tests covering the min_element/max_element issue) and confirm this doesn't cause compiler time tracking regressions.

This revision now requires changes to proceed.Mar 22 2021, 11:53 AM

As suggested by reviewers, limit usage to arrays with the same size and increase test coverage

RKSimon accepted this revision.Mar 23 2021, 3:17 AM

LGTM - cheers

This revision is now accepted and ready to land.Mar 23 2021, 3:17 AM
This revision was landed with ongoing or failed builds.Mar 23 2021, 6:23 AM
This revision was automatically updated to reflect the committed changes.