[PATCH] D111627: [DebugInfo][InstrRef] Recover some performance by calculating IDF for register units instead of all their alises

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando accepted this revision.
Orlando added a comment.
This revision is now accepted and ready to land.

LGTM! Plus a couple of inline nits.

> Testing: because this is performance related, no tests, but this builds clang-3.4 with identical object files after being applied (save for the object file with the date/time in it).

Out of interest do you have any numbers for the positive performance impact of this patch?



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1772-1773
+    bool AnyIllegal = false;
+    for (MCRegUnitIterator RUI(R.asMCReg(), TRI); RUI.isValid(); ++RUI) {
+      for (MCRegUnitRootIterator URoots(*RUI, TRI); URoots.isValid(); ++URoots){
+        if (!MTracker->isRegisterTracked(*URoots)) {
----------------
jmorse wrote:
> NB: this seems to be the canonical way of enumerating register unit... registers? in the rest of LLVM. What MCRegUnitIterator  produces doesn't seem to necessarily correspond to a single register, therefore there's an additional process of enumerating the roots too.
> 
> Either way, it works for x86, and this pattern is used elsewhere in llvm. So It Must Be Right (TM).
Thanks for the explanation. Nit: Please can you rename `URoots` to `URoot`?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1786
+    if (AnyIllegal) {
+      SlotsToPHI.insert(L); // WAS: put reg in regunitstophiup directly
+      continue;
----------------
nits: If you're also stuffing (arg) registers in too, maybe `SlotsToPHI` should be renamed. Something like `LocsToPHI` or `LocsWithoutRegUnits`?
Also is this a dev comment that leaked into the patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111627