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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/ADT/STLExtras.h:754
+  }
+};
+
----------------
rtereshin wrote:
> 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.
I've just had a look at the implementation of not_fn, and I think it's overkill for what we need it for. This implementation is consistent with the less_first and deref functions defined earlier in this file.


================
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);
   }
----------------
rtereshin wrote:
> 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
Good point. I'll make this change shortly


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:361
+  {
+    std::reverse(DwarfRegNums.begin(), DwarfRegNums.end());
+    std::stable_sort(DwarfRegNums.begin(), DwarfRegNums.end(),
----------------
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()


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:376
+    auto Last = std::unique(DwarfRegNums.begin(), DwarfRegNums.end(),
+                            on_first<equal>());
+    DwarfRegNums.erase(Last, DwarfRegNums.end());
----------------
rtereshin wrote:
> 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.
Looking at this code again, there's a few things that are questionable about it (and a couple we haven't already discussed off-list). 

LessRecordRegister is sorting by Record::getName(). The order is a bit weird but that's not particularly important here*, the important bit is that it's an order over Record::getName(). Record::getName() is unique for each Record since tablegen requires unique names and will synthesize unique names them for anonymous records. That would make it impossible to have duplicates were it not for RegisterTuple<>. RegisterTuple<> synthesizes new Record objects and derives their name from the subregisters. As a result, duplicate `(add R1, R2)` sequences in a RegisterTuple will break the uniqueness of the Record names.

However, the comments for RegisterTuple says "Most fields from the Register class are inferred, and the AsmName and Dwarf numbers are cleared." so RegisterTuple<> to RegisterTuple<> conflicts don't matter since the DwarfRegNum vector will be the same anyway. The only case I can see being a problem is when a Record synthesized from a RegisterTuple<> conflicts with a user-provided Record.

---
`*` At first glance, it looks like it's a a normal comparison except that it sorts numbers numerically, but it actually compares all the alpha components first, then all the numeric components. It also compares the length on the numeric strings before the value. As a result "A2A" < "A2B" and "A2A" < "A01A"


================
Comment at: utils/TableGen/RegisterInfoEmitter.cpp:453
+           "Expected Alias to be present in map");
+    DwarfRegNums.emplace_back(Reg, Tmp->second);
   }
----------------
rtereshin wrote:
> 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.
Well spotted. Looking at this again, there's another thing I missed on the first pass. This is handling the DwarfAlias field which can only be set for user-defined registers (and not the synthesized ones). We already added all the registers to the map on line 356 so we don't need to emplace. We should replace the existing pair instead


Repository:
  rL LLVM

https://reviews.llvm.org/D50272