[PATCH] D68209: [LiveDebugValues] Introduce entry values of unmodified params

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This looks good -- some minor nits inline. Keeping the backup-locations in the live-out sets as special records seems like a good solution for avoiding unwanted control flow effects, as shown with the diamond pattern.

Would I be right in thinking that the OpenRangesSet::erase method will now only erase entry value backup locations if you pass in a VarLoc that's also a backup location? This seems important, so that you can call erase and insert when a location _move_ happens, without disturbing the backup value. If this is the case, it should probably go into a comment somewhere documenting why two different sets of Vars / EntryValuesBackupVars are kept, it could be missed otherwise. (If this is documented, I missed the comment, oops).



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:93
+// If @Op is a stack or frame register return true, otherwise return false. This
+// is used to avoid the debug entry values from the registers, since we do not
+// support it at the moment.
----------------
"... to avoid basing the debug entry values on the registers..." ?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:388
+        return Loc.RegNo;
+      return 0;
+    }
----------------
Seeing how VarLocIDs is just a UniqueVector, isn't zero a legitimate index? I'm not aware of the first element being initialized to anything specific.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:524-526
+      assert((Vars.empty() && EntryValuesBackupVars.empty()) ==
+                 VarLocs.empty() &&
+             "open ranges are inconsistent");
----------------
Won't having "Vars.empty()" as a clause here mean the assertion fires if the OpenRanges isn't empty, rather than testing whether it's empty?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:809-811
+  if (I == MI.getParent()->rend() || I->isDebugInstr()) {
+    // This is a propagated DBG_VALUE.
+    return false;
----------------
If I understand this, if the immediately preceeding instruction is a debug instruction, it's considered to be propagated? This seems unreliable to me -- DBG_VALUEs often appear in packs.


================
Comment at: llvm/test/DebugInfo/MIR/X86/entry-values-diamond-bbs.mir:9-14
+# CHECK: bb.1.if.then
+# CHECK: DBG_VALUE $esi, $noreg, ![[ARG_B]], !DIExpression(DW_OP_LLVM_entry_value, 1)
+# CHECK: bb.2.if.else
+# CHECK: DBG_VALUE $esi, $noreg, ![[ARG_B]], !DIExpression(DW_OP_LLVM_entry_value, 1)
+# CHECK: bb.3.if.end
+# CHECK: DBG_VALUE $esi, $noreg, ![[ARG_B]], !DIExpression(DW_OP_LLVM_entry_value, 1)
----------------
IMHO more context should be checked for the entry value DBG_VALUEs, i.e. that they come after the correct instruction. The movement of "b" through these blocks is non-trivial, this test should make sure the entry values land in the right place as well as the right block. Same with the test below.


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

https://reviews.llvm.org/D68209