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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 5 inline comments as done.
djtodoro added a comment.

@jmorse Thanks for your comments! I tried to address them all!



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:388
+        return Loc.RegNo;
+      return 0;
+    }
----------------
jmorse wrote:
> 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.
I this this comments stands for `getEntryValueBackup()`, and I have addressed it. Thanks!


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:524-526
+      assert((Vars.empty() && EntryValuesBackupVars.empty()) ==
+                 VarLocs.empty() &&
+             "open ranges are inconsistent");
----------------
jmorse wrote:
> 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?
I am not sure any more, since this is not used at the current code..


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:809-811
+  if (I == MI.getParent()->rend() || I->isDebugInstr()) {
+    // This is a propagated DBG_VALUE.
+    return false;
----------------
jmorse wrote:
> 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.
Yes, I was not happy with the piece of code. I was gonna put a `HACK` marker above this, but I think it can be a `TODO` for now.


================
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)
----------------
jmorse wrote:
> 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.
I agree. I have addressed this.


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

https://reviews.llvm.org/D68209