[PATCH] D83047: [LiveDebugValues] 2/4 Add instruction-referencing LiveDebugValues implementation

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:138
+
+#include "llvm/ADT/CoalescingBitVector.h"
+#include "llvm/ADT/DenseMap.h"
----------------
Hopefully there's no need for CoalescingBitVector.h :)


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:227
+
+#define NUM_LOC_BITS 24
+
----------------
This "NUM_LOC_BITS" looks to be quite leaky -- I expect it'll save some pain to simply bite the bullet now and turn LocIdx into a real class / value type. Then all the asserts for 'Idx < (1<<24)' can be moved into the constructor.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:242
+  uint64_t BlockNo : 20;       /// The block where the def happens.
+  uint64_t InstNo : 20;        /// The Instruction where the def happens.
+                               /// One based, is distance from start of block.
----------------
Could you make `{Inst,Block}No` private, and introduce predicates like `isPhi()` instead of writing `P.InstNo == 0`?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:430
+    for (unsigned ID = 1; ID < LocIdxToIDNum.size(); ++ID) {
+      LocIdxToIDNum[LocIdx(ID)] = ValueIDNum::fromU64(Locs[ID]);
+    }
----------------
In `ValueIDNum::fromU64(Locs[ID])`, where does the reset to InstNo = 0 happen? Maybe this would be clearer if we could write `<someValueIDNum>.setIsPhi(true)`?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:448
+    // SpillsToMLocs.reset(); XXX can't reset?
+    SpillLocs = decltype(SpillLocs)();
+
----------------
UniqueVector does have  a 'reset' method. Does it not work?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:463
+  /// a live-in, or a recent register mask clobber.
+  void bumpRegister(unsigned ID, LocIdx &Ref) {
+    assert(ID != 0);
----------------
Is there some name more specific than "bumpRegister" that conveys "get a LocIdx for a register ID"?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:463
+  /// a live-in, or a recent register mask clobber.
+  void bumpRegister(unsigned ID, LocIdx &Ref) {
+    assert(ID != 0);
----------------
vsk wrote:
> Is there some name more specific than "bumpRegister" that conveys "get a LocIdx for a register ID"?
Why does "Ref" have to be an in-out parameter? Is there some way to simply return a correct LocIdx?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:472
+      // Was this reg ever touched by a regmask?
+      for (auto Rit = Masks.rbegin(); Rit != Masks.rend(); ++Rit) {
+        if (Rit->first->clobbersPhysReg(ID)) {
----------------
`const auto &MaskPair : reverse(Masks)`?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:533
+    // Ensure SP exists, so that we don't override it later.
+    unsigned SP = TLI.getStackPointerRegisterToSaveRestore();
+    if (SP) {
----------------
Can the LocIdx for SP be initialized when the MLocTracker is initialized?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:540
+
+    // Def anything we already have that isn't preserved.
+    for (auto &P : LocIdxToLocID) {
----------------
I see - so here, "kill the register" is represented by a def.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:728
+  /// the order in this thing.
+  MapVector<DebugVariable, DbgValue> Vars;
+  DenseMap<DebugVariable, const DILocation *> Scopes;
----------------
Could you leave a short blurb here explaining why it's sufficient to keep the last DbgValue for a DebugVariable?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:871
+      if (it == ValueToLoc.end() || MTracker->isSpill(it->second) ||
+          !isCalleeSaved(it->second))
+        ValueToLoc[VNum] = LocIdx(Idx);
----------------
Do we care to prefer a spill over a non-callee saved reg?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:920
+    if (It != ActiveVLocs.end()) {
+      ActiveMLocs[It->second.first].erase(Var);
+    }
----------------
I find it difficult to parse It->second.first -- would you mind making LocAndProperties a struct, so it's clear that this is accessing a location?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:1166
+  /// number, the inner by LocIdx.
+  void mlocDataflow(uint64_t **MInLocs, uint64_t **MOutLocs,
+                    SmallVectorImpl<MLocTransferMap> &MLocTransfer);
----------------
What makes it necessary to write this in terms of uint64_t, instead of ValueIDNum?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:360
+  /// as the number of registers on the target -- if the value in the register
+  /// is not being tracked, then the LocIdx value will be zero. New entries are
+  /// appended if a new spill slot begins being tracked.
----------------
jmorse wrote:
> vsk wrote:
> > Why does there need to be a LocIdx reserved for the case where the value in a register isn't tracked? It doesn't look like this is done for stack slots.
> Ah, this is the curse of early optimisation: it's a shortcut so that LocIDToLocIdx can be an array with constant-time lookup to identify whether the corresponding register is tracked or not (signalled by the element being nonzero). This could easily be eliminated by making LocIDToLocIdx an associative array. It isn't necessary for stack slots as they're identified by an associative array elsewhere.
> 
> However, outside of MLocTracker, I've been using a zero LocIdx as shorthand for "this is an invalid/empty value/location", in the manner of Optional<> and None. It's not really tied to either registers or stack slots.
> 
> This isn't good practice, but there was a stage in prototyping where the machine value number live-in / live-outs of a block could be an empty or null value.  I don't think this is the case any more (95% sure) , I'll spend some time polishing this "feature" out of the implementation.
That would be great - imo there's a significant long-term benefit to not having a representation for invalid state. (Other than Optional<> of course.)


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

https://reviews.llvm.org/D83047