[PATCH] D83890: [DebugInfo] Process DBG_VALUE_LIST in LiveDebugValues

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

Thanks for doing this!

Can you please update the top-level comment of the file, so we have the whole picture of the change?



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:263
+// Simple Set for storing all the VarLoc Indices at a Location bucket.
+using VarLocsInRange = SmallSet<LocIndex::u32_index_t, 32>;
+// Vector of all `LocIndex`s for a given VarLoc; the same Location should not
----------------
In stands for Indices here? I'd change that in order not to mix up terms with the In/Out...


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:267
+// Location bucket.
+using LocIndices = SmallVector<LocIndex, 2>;
+
----------------
This should have more intuitive name, I guess...


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:308
 
-    enum VarLocKind {
+    enum class MachineLocKind {
       InvalidKind = 0,
----------------
I don't understand why we need new Kind for the EntryValue here.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:322
 
-    /// The value location. Stored separately to avoid repeatedly
-    /// extracting it from MI.
-    union {
+    typedef union {
       uint64_t RegNo;
----------------
why typedef here?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:331
+
+    struct MachineLoc {
+      MachineLocKind Kind;
----------------
Please add a comment here by describing the purpose of the structure.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1579
+        unsigned SpillLocIdx = VL.getSpillLocIdx(*Loc);
+        // const MachineOperand &MO = VL.MI.getDebugOperand(SpillLocIdx);
+        // VarLoc::MachineLoc OldLoc = VarLoc::GetLocForOp(MO);
----------------
This should be deleted?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:1719
   bool Changed = false;
-
-  LLVM_DEBUG(for (uint64_t ID
-                  : OpenRanges.getVarLocs()) {
-    // Copy OpenRanges to OutLocs, if not already present.
-    dbgs() << "Add to OutLocs in MBB #" << CurMBB->getNumber() << ":  ";
-    VarLocIDs[LocIndex::fromRawInteger(ID)].dump(TRI);
+  using VarVec = SmallVector<VarLoc, 32>;
+  LLVM_DEBUG({
----------------
We have declared this multiple times, therefore, can we declare it just once out of the methods?


================
Comment at: llvm/test/DebugInfo/MIR/X86/dvl-livedebugvalues-clobber.mir:73
+name:            _Z3fooii
+alignment:       16
+exposesReturnsTwice: false
----------------
We don't need all the MF attributes. Most of them could be deleted (applies to all the tests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83890