This is an archive of the discontinued LLVM Phabricator instance.

[llvm-exegesis] InstructionBenchmarkClustering::rangeQuery(): use llvm::SmallVector<size_t, 0> for storage.
ClosedPublic

Authored by lebedev.ri on Nov 11 2018, 1:01 AM.

Details

Summary

Old: (D54383)

 Performance counter stats for './bin/llvm-exegesis -mode=analysis -analysis-epsilon=100000 -benchmarks-file=/tmp/benchmarks.yaml -analysis-inconsistencies-output-file=/tmp/clusters.html' (10 runs):

       9098.781978      task-clock (msec)         #    1.000 CPUs utilized            ( +-  0.16% )
...
            9.1015 +- 0.0148 seconds time elapsed  ( +-  0.16% )

New:

 Performance counter stats for './bin/llvm-exegesis -mode=analysis -analysis-epsilon=100000 -benchmarks-file=/tmp/benchmarks.yaml -analysis-inconsistencies-output-file=/tmp/clusters.html' (10 runs):

       8553.352480      task-clock (msec)         #    1.000 CPUs utilized            ( +-  0.12% )
...
            8.5539 +- 0.0105 seconds time elapsed  ( +-  0.12% )

So another -6%.
That is because the SmallVector doubles it size when reallocating, which is great here,
since we can't reserve() since we can't know how many Neighbors we will have.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Nov 11 2018, 1:01 AM
MaskRay requested changes to this revision.Nov 11 2018, 5:09 PM
MaskRay added inline comments.
tools/llvm-exegesis/lib/Clustering.cpp
38

The size of Neighbors is bound by the number of points. The vector can be reserved before the loop calling rangeQuery and passed as a parameter into rangeQuery.

This revision now requires changes to proceed.Nov 11 2018, 5:09 PM
lebedev.ri added inline comments.Nov 11 2018, 11:16 PM
tools/llvm-exegesis/lib/Clustering.cpp
38

Good point.

That is the upper bound, not in all cases that will be actual size of the vector.
Given that you have accepted D54390, such optimization can be safely made
but only *after* D54390 (and it can be replaced by a std::vector),
but i really really don't want to change/reorder patches,
because that would require to re-do all of the benchmarks/measurements.

So i will add a new patch with that fix.

You don't need this one anymore right ? Though I don't oppose to you keeping it for benchmarking reference.

You don't need this one anymore right ? Though I don't oppose to you keeping it for benchmarking reference.

Correct. It has been effectively superseeded by further patches, but it would make sense to keep it for consistency, as a fine-grained refactoring step.
Thank you for the review!

This revision was not accepted when it landed; it landed in state Needs Revision.Nov 19 2018, 5:31 AM
This revision was automatically updated to reflect the committed changes.