[PATCH] D54418: [llvm-exegesis] InstructionBenchmarkClustering::dbScan(): replace SetVector with custom BitVectorVector

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In https://reviews.llvm.org/D54418#1296615, @lebedev.ri wrote:

> In https://reviews.llvm.org/D54418#1296093, @MaskRay wrote:
>
> > Thanks for your benchmark file!
> >
> > `llvm-exegesis -mode=analysis -analysis-epsilon=100000 -benchmarks-file=/tmp/Downloads/benchmarks.yaml -analysis-inconsistencies-output-file=/tmp/clusters.html > /tmp/new`
> >
> > https://reviews.llvm.org/D54442 alone (without your other optimization) reduces the time from 16s to 4.2s on my machine. I'll leave the  `const auto Neighbors = rangeQuery(Q);` inlining and other stuff to you :)
>
>
> After thinking, i see what the question was - why use abstractions when you can avoid using abstractions.
>  Well, because they are abstractions :) I have seen a bit much code, than i would have liked to, that intentionally avoided abstractions.
>  So i personally would not write everything inline like it is in https://reviews.llvm.org/D54442, and would not review that.
>  If it is abstracted, it is more readable, actually testable. And one day might even be moved to a more common (ADT e.g.) place should others need it.
>  If it is all inline, someone *will* have to refactor it to use abstractions later on.
>  So i guess the question boils down to - why bother writing good code when one can write "C spaghetti".
>
> I'm not sure if https://reviews.llvm.org/D54442 is just a proof-of-concept or actually for the review,
>  but i would personally guess that small incremental patches
>  (with actual perf changes stated! since it is a perf improvements patchset) are better,
>  since small incremental changes are anyway recommended by the coding standards.


https://reviews.llvm.org/D54442 is also for review. It uses inlined bit vector `Added` (`vector<char>` is faster than `BitVector` in this case) as I don't think the abstraction is really necessary. It is used similarly as what would be used in BFS or Dijkstra's algorithm.


Repository:
  rL LLVM

https://reviews.llvm.org/D54418