[PATCH] D87357: [SelectionDAG][DebugInfo] Use entry-values to recover parameters values

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer accepted this revision.
StephenTozer added a comment.
This revision is now accepted and ready to land.

LGTM, this new approach looks solid. As per usual, wait for further approval from someone with more authority in this area.



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:1477
                           unsigned R, bool IsIndirect, const DebugLoc &DL,
-                          unsigned O);
+                          unsigned O, unsigned vRegForEntryValue = 0,
+                          DIExpression *EntryExpr = nullptr);
----------------
Nit, I think the "v" should be uppercase.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:710
   if (SD->isInvalidated()) {
+    Register Reg = SD->getVRegForEntryValue();;
+    DIExpression *EntryValueExpr = SD->getEntryExpression();
----------------
Double semi-colon here.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:722-724
     // An invalidated SDNode must generate an undef DBG_VALUE: although the
     // original value is no longer computed, earlier DBG_VALUEs live ranges
     // must not leak into later code.
----------------
Minor: Comment could do with a small rewrite, something like "must generate an undef" -> "must generate a DBG_VALUE that is either undef, or an entry value if one is available"


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SDNodeDbgValue.h:68-69
     u.s.Node = N;
     u.s.ResNo = R;
+    VRegForEntryValue = vRegForEntryValue;
   }
----------------
Nit, this can be put in the initializer list.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5482-5484
+    // info recovering.
+    if (FuncInfo.ArgValueMap.find(V) == FuncInfo.ArgValueMap.end())
+      FuncInfo.ArgValueMap[V] = Reg;
----------------
Perhaps instead of simply making this a condition, there should be an assert here that `V` doesn't already exist in `ArgValueMap`? I can't think of any case where it would already be there that wouldn't be an error, since I don't think we would ever call this twice for the same Value.


================
Comment at: llvm/test/DebugInfo/X86/entry-val-invalidated-node-modifed-param-with-add.ll:1-3
+; RUN: llc < %s -O2 -stop-before=finalize-isel | FileCheck %s
+; RUN: llc -O2 %s -o %t -filetype=obj
+; RUN: llvm-dwarfdump %t | FileCheck %s --check-prefix=CHECK-DWARFDUMP
----------------
Personally I think it'd probably be best to specify the DWARF version in this, so that this test doesn't break if the default becomes 5 (or already is in a clone repo). It might be better to simply update the test if/when that happens though, I don't have especially strong opinions about it. Alternatively you could simply regex it as `DW_OP{{(_GNU)?}}_entry_value`; also not sure whether or not this approach would be preferred.


================
Comment at: llvm/test/DebugInfo/X86/entry-values-for-isel-invalidated-nodes.ll:1-4
+; RUN: llc < %s -O2 -stop-before=finalize-isel | FileCheck %s
+; RUN: llc -O2 %s -o %t -filetype=obj
+; RUN: llvm-dwarfdump %t | FileCheck %s --check-prefix=CHECK-DWARFDUMP
+
----------------
Repeat of above comment about DWARF version.


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

https://reviews.llvm.org/D87357