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

Thu Jul 23 13:17:26 PDT 2020

jmorse added a comment.

[Bah, comments didn't get submitted]

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

================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:227
+
+#define NUM_LOC_BITS 24
+
----------------
vsk wrote:
> 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.
I've made this change; I've also moved the two maps indexed by LocIdx's in MLocTracker away from being plain vectors. They're now IndexedMaps, which are vectors underneath, but now have a tiny little bit more type safety about them.

There are now more places where we enumerate all locations by for-loop-and-counter over the contents of LocIdxToIDNum: I'd really enjoy having a locations() method to generate a range... however I'm not aware of C++ being able to describe integer ranges until C++20 :/

================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:278
+
+// Boilerplate densemapinfo for ValueIDNum.
+namespace llvm {
----------------
djtodoro wrote:
> we can delete this now?
Sure -- this moves the "ActiveMLocs" container in TransferTracker back to being a std::map. As I think Vedant pointed out, if this turns out to be a performance thing, we can restore it some other time.

================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:430
+    for (unsigned ID = 1; ID < LocIdxToIDNum.size(); ++ID) {
+      LocIdxToIDNum[LocIdx(ID)] = ValueIDNum::fromU64(Locs[ID]);
+    }
----------------
vsk wrote:
> 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)`?
Ugh; this was actually an out-of-date comment, now updated.

================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:448
+    // SpillsToMLocs.reset(); XXX can't reset?
+    SpillLocs = decltype(SpillLocs)();
+
----------------
vsk wrote:
> UniqueVector does have  a 'reset' method. Does it not work?
It's implemented within UniqueVector as `Vector.resize(0, 0)`, which appears to assume that the mapped type can be initialized with a zero. I haven't put any thought into fixing it (yet) as it could be worked around.

================
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:
> 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?
I've revised this to better reflect what's really going on: this method really exists to handle tracking of a new register, and I've added a `lookupOrTrackRegister` method to abstract over just looking up a register.

================
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) {
----------------
vsk wrote:
> Can the LocIdx for SP be initialized when the MLocTracker is initialized?
Good idea; I've folded that into the constructor.

================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:540
+
+    // Def anything we already have that isn't preserved.
+    for (auto &P : LocIdxToLocID) {
----------------
vsk wrote:
> I see - so here, "kill the register" is represented by a def.
Yeah, my understanding of register masks is that they end the liveness of a register, which means that the registers value can't be relied upon. That means we have to over-write the value number with something -- originally I used the LocIdx(0) "empty value" indicator, but def'ing them works just as well.

================
Comment at: llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp:871
+      if (it == ValueToLoc.end() || MTracker->isSpill(it->second) ||
+          !isCalleeSaved(it->second))
+        ValueToLoc[VNum] = LocIdx(Idx);
----------------
vsk wrote:
> Do we care to prefer a spill over a non-callee saved reg?
(I've added this -- I don't have a useful setup for measuring the effects of this right now, alas).

================
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);
----------------
vsk wrote:
> What makes it necessary to write this in terms of uint64_t, instead of ValueIDNum?
Nothing -- I think it just matched my mental model of "here's a table of value numbers" to be mangled. I'll switch to writing this as ValueIDNum, but in the interests of readable patches I'll do that in an additional revision.

================
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.
----------------
vsk wrote:
> 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.)
(I'm still planning on this, slowly)

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

https://reviews.llvm.org/D83047