[PATCH] D152497: [SetVector] Implement better "small" functionality

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Apart from the note on the static_assert, this looks good to me. There's a significant compile-time improvement, a minor memory usage improvement, and even the size of the clang binary gets slightly smaller, so this seems like an improvement on all metrics for relatively little additional complexity.



================
Comment at: llvm/include/llvm/ADT/SetVector.h:50
+          typename Set = DenseSet<T>, unsigned N = 0>
 class SetVector {
 public:
----------------
It might make sense to replicate this static_assert from SmallPtrSet: https://github.com/llvm/llvm-project/blob/ecbd37d5a336ec8a8adafae5a8e4263bb738718c/llvm/include/llvm/ADT/SmallPtrSet.h#L451-L454

To make sure people don't shoot themselves in the foot with a nice `SmallSetVector<T, 512>`.

>From a quick grep, this is the only usage that would have to be adjusted: https://github.com/llvm/llvm-project/blob/ecbd37d5a336ec8a8adafae5a8e4263bb738718c/flang/lib/Lower/Bridge.cpp#L190


================
Comment at: llvm/include/llvm/ADT/SetVector.h:202-209
+    if constexpr (canBeSmall())
+      if (isSmall())
+        return vector_.erase(I);
+
     const key_type &V = *I;
     assert(set_.count(V) && "Corrupted SetVector instances!");
     set_.erase(V);
----------------
Could write it like this, not sure if better.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152497/new/

https://reviews.llvm.org/D152497