[Mlir-commits] [clang] [clang-tools-extra] [flang] [lld] [llvm] [mlir] [polly] [SetVector] Make SetVector::insert consistent (NFC) (PR #232372)

[Eugene Epshteyn]

================
@@ -153,23 +153,27 @@ class SetVector {
   }
 
   /// Insert a new element into the SetVector.
-  /// \returns true if the element was inserted into the SetVector.
-  bool insert(const value_type &X) {
+  /// \returns a pair, whose first member is the iterator pointed to the
+  /// element, and the second member indicates if the element was inserted.
+  std::pair<iterator, bool> insert(const value_type &X) {
     if constexpr (canBeSmall())
       if (isSmall()) {
-        if (!llvm::is_contained(vector_, X)) {
+        auto It = find(vector_, X);
+        if (It == end()) {
           vector_.push_back(X);
           if (vector_.size() > N)
             makeBig();
-          return true;
+          return {std::prev(vector_.end()), true};
         }
-        return false;
+        return {It, false};
       }
 
     bool result = set_.insert(X).second;
-    if (result)
+    if (result) {
       vector_.push_back(X);
-    return result;
+      return {std::prev(vector_.end()), result};
+    }
+    return {find(vector_, X), result};
----------------
eugeneepshteyn wrote:

(This comment was generated with the help of AI)

This `find` is what the compile-time tracker measures (about +0.06% geomean at stage1 and stage2 `-O3`, ThinLTO and LTO, and +0.14% instructions to build clang). It is dead only when `insert` is inlined into a caller that ignores `.first`. Whenever an instantiation stays out of line, the pair must be materialized and every rejected insert scans the whole vector. That depends on inliner size heuristics, so neither clang nor gcc can be relied on to remove it.

A clang `-O3` build of this branch has such out-of-line copies; for example `SetVector<unsigned, SmallVector<unsigned, 0>, DenseSet<unsigned>>::insert` in `fir-opt` runs the find loop on its duplicate path. `-O2` (RelWithDebInfo) and Debug builds pay the cost as well.

```cpp
#include "llvm/ADT/SetVector.h"
#include <chrono>
#include <cstdio>
#include <cstdlib>
#include <vector>

#ifdef BASE
#define INSERTED(S, X) (S).insert(X)
#else
#define INSERTED(S, X) (S).insert(X).second
#endif

int main(int argc, char **argv) {
  unsigned N = argc > 1 ? std::atoi(argv[1]) : 4000;
  std::vector<int> Storage(N);
  llvm::SetVector<int *> S;
  for (int &V : Storage)
    INSERTED(S, &V);
  unsigned Dups = 0;
  auto T0 = std::chrono::steady_clock::now();
  for (unsigned R = 0; R < 50; ++R)
    for (int &V : Storage)
      Dups += !INSERTED(S, &V);
  auto T1 = std::chrono::steady_clock::now();
  std::printf("N=%u: %.1f ns per duplicate insert\n", N,
              std::chrono::duration<double, std::nano>(T1 - T0).count() / Dups);
}
```

Built with clang 21, once against `main` with `-DBASE` and once against this PR (`clang++ -std=c++17 <flags> -I llvm/include -I build/include dup-insert-cost.cpp build/lib/libLLVMSupport.a`), ns per duplicate insert:

| flags | N | main | this PR |
|---|---|---|---|
| `-O2` | 1000 | 2.0 | 82.4 |
| `-O2` | 16000 | 2.1 | 1151.0 |
| `-O3` | 16000 | 1.2 | 1.2 |
| `-O3 -fno-inline` | 1000 | 36.0 | 606.7 |
| `-O3 -fno-inline` | 16000 | 37.8 | 8225.3 |

`insert` should keep its cheap `bool` path. A caller that needs the position can use `MapVector<T, unsigned>`, or a separately named lookup that documents its cost.


https://github.com/llvm/llvm-project/pull/232372