This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Transfer DbgValues when integer operations are promoted
ClosedPublic

Authored by asmith on Mar 15 2018, 4:05 PM.

Details

Summary

DbgValue nodes were not transferred when integer DAG nodes were promoted. For example, if an i32 add node was promoted to an i64 add node by DAGTypeLegalizer::PromoteIntegerResult(), its DbgValue node was not transferred to the new node. The simple fix is to update SetPromotedInteger() to transfer DbgValues.

Add AArch64/dbg-value-i8.ll to test this change and fix ARM/debug-info-d16-reg.ll which had the wrong DILocalVariable nodes with arg numbers even though they are not for function parameters.

Patch by Se Jong Oh!

Diff Detail

Event Timeline

asmith created this revision.Mar 15 2018, 4:05 PM

Could you please add a test case for this?

This is to fix DebugInfo/Generic/linear-dbg-value.ll and won't repro on an in-tree target.

Is there a way to write a test that will force selection dag to promote an integer?

vsk added a comment.Mar 15 2018, 4:36 PM

This is to fix DebugInfo/Generic/linear-dbg-value.ll and won't repro on an in-tree target.

Is there a way to write a test that will force selection dag to promote an integer?

We can probably find a test by adding an assertion that catches interesting input, and throwing lots of debug info at the backend. E.g:

diff --git a/include/llvm/CodeGen/SelectionDAG.h b/include/llvm/CodeGen/SelectionDAG.h
index 0df75cb0b1d..5c2fc3bee5e 100644
--- a/include/llvm/CodeGen/SelectionDAG.h
+++ b/include/llvm/CodeGen/SelectionDAG.h
@@ -1228,7 +1228,7 @@ public:
   /// Transfer debug values from one node to another, while optionally
   /// generating fragment expressions for split-up values. If \p InvalidateDbg
   /// is set, debug values are invalidated after they are transferred.
-  void transferDbgValues(SDValue From, SDValue To, unsigned OffsetInBits = 0,
+  bool transferDbgValues(SDValue From, SDValue To, unsigned OffsetInBits = 0,
                          unsigned SizeInBits = 0, bool InvalidateDbg = true);
 
   /// Remove the specified node from the system. If any of its
diff --git a/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp b/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
index 4438ee7878b..03eab5cd94e 100644
--- a/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
+++ b/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
@@ -776,6 +776,9 @@ void DAGTypeLegalizer::SetPromotedInteger(SDValue Op, SDValue Result) {
   SDValue &OpEntry = PromotedIntegers[Op];
   assert(!OpEntry.getNode() && "Node is already promoted!");
   OpEntry = Result;
+
+  bool FoundIt = DAG.transferDbgValues(Op, Result);
+  assert(!FoundIt && "Found a test case");
 }
 
 void DAGTypeLegalizer::SetSoftenedFloat(SDValue Op, SDValue Result) {
diff --git a/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index aac79075e6e..ea6e151dad2 100644
--- a/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -7187,7 +7187,7 @@ SDDbgValue *SelectionDAG::getFrameIndexDbgValue(DIVariable *Var,
   return new (DbgInfo->getAlloc()) SDDbgValue(Var, Expr, FI, DL, O);
 }
 
-void SelectionDAG::transferDbgValues(SDValue From, SDValue To,
+bool SelectionDAG::transferDbgValues(SDValue From, SDValue To,
                                      unsigned OffsetInBits, unsigned SizeInBits,
                                      bool InvalidateDbg) {
   SDNode *FromNode = From.getNode();
@@ -7198,10 +7198,10 @@ void SelectionDAG::transferDbgValues(SDValue From, SDValue To,
   // TODO: assert(From != To && "Redundant dbg value transfer");
   // TODO: assert(FromNode != ToNode && "Intranode dbg value transfer");
   if (From == To || FromNode == ToNode)
-    return;
+    return false;
 
   if (!FromNode->getHasDebugValue())
-    return;
+    return false;
 
   SmallVector<SDDbgValue *, 2> ClonedDVs;
   for (SDDbgValue *Dbg : GetDbgValues(FromNode)) {
@@ -7242,6 +7242,7 @@ void SelectionDAG::transferDbgValues(SDValue From, SDValue To,
 
   for (SDDbgValue *Dbg : ClonedDVs)
     AddDbgValue(Dbg, ToNode, false);
+  return ClonedDVs.size();
 }
 
 void SelectionDAG::salvageDebugInfo(SDNode &N) {

Here's a test that trips the assert:

$ ./bin/llvm-lit /Users/vsk/src/llvm.org-debugify/llvm/test/CodeGen/ARM/debug-info-d16-reg.ll -a
Assertion failed: (!FoundIt && "Found a test case"), function SetPromotedInteger

You could probably modify this for your purposes.

Thanks, that's helpful.

asmith updated this revision to Diff 138790.Mar 16 2018, 3:52 PM
asmith edited the summary of this revision. (Show Details)
aprantl added inline comments.Mar 16 2018, 3:57 PM
test/DebugInfo/AArch64/dbg-value-i8.ll
8

Can you run llc as llc -stop-after=livedebugvariables and check for a DBG_VALUE instead? This should make the test more robust.

29

Please remove all attributes that are not necessary to reproduce the test. (Usually everything in quotes).

asmith updated this revision to Diff 138801.Mar 16 2018, 5:35 PM
asmith marked 2 inline comments as done.
JDevlieghere accepted this revision.Mar 19 2018, 4:58 AM

Unless Adrian has any more remarks, this LGTM. Thank you!

This revision is now accepted and ready to land.Mar 19 2018, 4:58 AM
This revision was automatically updated to reflect the committed changes.