[PATCH] D38068: [DebugInfo] Use a DenseMap to coalesce MachineOperand locations

[Hans Wennborg via Phabricator via llvm-commits]

hans accepted this revision.
hans added a comment.

I'm not familiar with the DBG_VALUE stuff, but this looks good as far as I can tell.



================
Comment at: llvm/include/llvm/CodeGen/MachineOperand.h:787
+  enum : unsigned char {
+    MO_Empty = MO_Predicate + 1,
+    MO_Tombstone,
----------------
I wonder if this could be made future-proof in case another MO_ enumerator is ever added. Maybe add a MO_LAST = MO_Predicate to the enumeration?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:947
+  // IntervalMap if two vreg intervals collapse to the same physical location.
+  MapVector<MachineOperand, bool> NewLocations;
+  SmallVector<unsigned, 4> LocNoMap(locations.size());
----------------
nit: the change description said DenseMap :-)
Maybe comment somehow that the bool is just a dummy here and you're just using it as a set?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:970
+
+    // Insert the new location and record its number.
+    auto InsertResult = NewLocations.insert({Loc, false});
----------------
Should this comment on the fact that coalescing can happen here if the new location is identical to something already in NewLocations?


https://reviews.llvm.org/D38068