[PATCH] D50272: [tablegen] Improve performance of -gen-register-info by replacing barely-necessary std::map with a sorted vector

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin requested changes to this revision.
rtereshin added a comment.
This revision now requires changes to proceed.

This is an impressive result, thanks for doing this!



================
Comment at: include/llvm/ADT/STLExtras.h:754
+  }
+};
+
----------------
Maybe it makes sense to do this `not_fn`-style (https://en.cppreference.com/w/cpp/utility/functional/not_fn) so it could work with function pointers and lambda-functions, not just struct-like function objects with trivial constructors.

Of course it makes sense to do that only when actually needed, not necessarily now.


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:356
     maxLength = std::max((size_t)maxLength, RegNums.size());
-    if (DwarfRegNums.count(Reg))
-      PrintWarning(Reg->getLoc(), Twine("DWARF numbers for register ") +
-                   getQualifiedName(Reg) + "specified multiple times");
-    DwarfRegNums[Reg] = RegNums;
+    DwarfRegNums.emplace_back(Reg, RegNums);
   }
----------------
I feel like
```
DwarfRegNums.emplace_back(Reg, std::move(RegNums));  // I like to move it, move it
```
is going to make sense here: https://godbolt.org/g/rNN15d


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:361
+  {
+    std::reverse(DwarfRegNums.begin(), DwarfRegNums.end());
+    std::stable_sort(DwarfRegNums.begin(), DwarfRegNums.end(),
----------------
I think instead of doing this `std::reverse` we could just do `std::unique` below on `rbegin`/`rend` instead of `begin`/`end` and achieve the same result with less memory churn.


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:376
+    auto Last = std::unique(DwarfRegNums.begin(), DwarfRegNums.end(),
+                            on_first<equal>());
+    DwarfRegNums.erase(Last, DwarfRegNums.end());
----------------
Looks like the original `std::map` has uniqued the records by `!LessRecordRegister()(Reg, LastSeenReg) && !LessRecordRegister()(LastSeenReg, Reg)`, not by a pointer (see https://en.cppreference.com/w/cpp/container/map).

Maybe we could add another function object to `ADT/STLExtras.h`, something like
```
template<typename LessTy>
struct equivalent {
  LessTy less;

  template <typename A, typename B> bool operator()(A &&a, B &&b) const {
    return !less(std::forward<A>(a), std::forward<B>(b)) &&
           !less(std::forward<B>(b), std::forward<A>(a);
  }
};
```
(give or take) so we could call `std::unique(DwarfRegNums.begin(), DwarfRegNums.end(), on_first<equivalent<LessRecordRegister>>());`

More efficient way of achieving the same in this specific case I think is bringing in C++17's `not_fn` (https://en.cppreference.com/w/cpp/utility/functional/not_fn) and doing `std::unique(DwarfRegNums.begin(), DwarfRegNums.end(), not_fn(on_first<LessRecordRegister>()));`: if `a` is not less than `b` than `a` is equivalent `b` as the vector is sorted and `b` not less than `a` is already guaranteed.


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:453
+           "Expected Alias to be present in map");
+    DwarfRegNums.emplace_back(Reg, Tmp->second);
   }
----------------
I think this immediately breaks the "sorted" invariant required for `lower_bound` to work correctly. I think we need to remember the size of the `DwarfRegNums` before the loop and do `std::lower_bound(DwarfRegNums.begin(), DwarfRegNums.begin() + DwarfRegNumsSize, ...)` instead of `std::lower_bound(DwarfRegNums.begin(), DwarfRegNums.end(), ...` to fix this. The `std::sort` after the loop will be of course still required.


Repository:
  rL LLVM

https://reviews.llvm.org/D50272