[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 accepted this revision.
rtereshin added a comment.
This revision is now accepted and ready to land.

LGTM with minor nitpicks, please feel free to make changes you'll find reasonable while committing.



================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:361
+  {
+    std::reverse(DwarfRegNums.begin(), DwarfRegNums.end());
+    std::stable_sort(DwarfRegNums.begin(), DwarfRegNums.end(),
----------------
dsanders wrote:
> rtereshin wrote:
> > 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.
> I agree. I was also wondering if the reverse is necessary to begin with, I've only put it there to preserve behaviour. None of the in-tree targets are emitting this warning and it seems fairly arbitrary to pick the last definition over the first. I'm pretty sure we can delete it
> 
> Unfortunately, while unique() is happy with a reverse_iterator, you can't give the result to erase()
So do you want to delete it?

> Unfortunately, while unique() is happy with a reverse_iterator, you can't give the result to erase()
Sadly true, didn't expect that. 


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:458
+    const auto &RegIter =
+        std::lower_bound(DwarfRegNums.begin(), DwarfRegNums.end(), Reg,
+                         [](const DwarfRegNumsMapPair &A, const Record *B) {
----------------
I think we can get rid of this second `lower_bound` call if we iterate over `DwarfRegNums`, not over `Regs`.


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:464
+           "Expected Reg to be present in map");
+    *RegIter = std::make_pair(Reg, AliasIter->second);
   }
----------------
Why not just `RegIter->second = AliasIter->second;`?


Repository:
  rL LLVM

https://reviews.llvm.org/D50272