[PATCH] D80684: [LiveDebugValues] Speed up removeEntryValue, NFC

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 3 inline comments as done and an inline comment as not done.
vsk added a subscriber: tbosch.
vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:917
       const VarLoc &VL = VarLocIDs[LocIndex::fromRawInteger(ID)];
-      if (!VL.isEntryBackupLoc())
-        continue;
----------------
djtodoro wrote:
> vsk wrote:
> > djtodoro wrote:
> > > I'm not sure we want to remove this line. The backup locations are `EntryValueBackupKind` and `EntryValueCopyBackupKind` only; the `EntryValueKind` is real location created from one of these two.
> > It looks like `VL.getEntryValueCopyBackupReg()` returns undef when VL.Kind isn't the right kind. So it seems safe to remove the `VL.isEntryBackupLoc()` check?
> Oh, I see. It is safe! Thanks.
> Eventually, if we get rid of the [1] we won't need this part at all. The `EntryValueCopyBackupKind` was used to indicate the variable's value was not changed with the new DBG_VALUE (for the same variable), but it was moved around due to the code from [1].
> 
> [1] https://github.com/llvm/llvm-project/blob/446082b99f0bd9786d24edd3cd598dc4e5f64371/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp#L590
I see, thanks for the pointer (cc @tbosch, this is related to the discussion from D80819).


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1362
+             VL.Kind == VarLoc::SpillLocKind && "Broken VarLocSet?");
+      if (VL.Loc.SpillLocation == *Loc)
+        LLVM_DEBUG(dbgs() << "Restoring Register " << printReg(Reg, TRI) << '('
----------------
aprantl wrote:
> aprantl wrote:
> > ```
> > if (VL.Loc.SpillLocation != *Loc)
> >   // Comment explains why no pair is inserted...
> >   continue;
> > LLVM_DEBUG(dbgs() << "Restoring Register " ...
> > ```
> did this get marked as done by accident?
Sorry about that, somehow I missed that you were asking to convert this to an early exit. Should be fixed soon.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:477
     /// within Loc2Vars.
     std::map<VarLoc, uint32_t> Var2Index;
 
----------------
aprantl wrote:
> Side-note: does this get more readable if we introduce a typedef for uint32_t here, such a `VarLocIndex`?
That may look a little too similar to "LocIndex". Wdyt of `LocIndex::uint32_location_t` and `LocIndex::uint32_index_t`?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:493
+      case VarLoc::EntryValueKind:
+      case VarLoc::EntryValueBackupKind:
+      case VarLoc::EntryValueCopyBackupKind:
----------------
djtodoro wrote:
> Should we make a different `kEntryValBackupLocation` (and something like `getBackupEntryValuesVarLocs()`)?
> 
> The backup locations (`EntryValueBackupKind` and `EntryValueCopyBackupKind`) are invisible/backup/secondary up to the point we lost the primary location of a variable; and then we turn the backup location into entry value location (`EntryValueKind`) which is (further on) used as primary location.
Good point, this should now be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80684