[PATCH] D55227: [DebugInfo] Don't drop dbg.value's of nullptr

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
jmorse added reviewers: aprantl, vsk, probinson, dstenb.
Herald added a subscriber: llvm-commits.

Currently, dbg.value's of "nullptr" are dropped when entering a SelectionDAG -- apparently just because of an oversight when recognising Values that are constant (see PR39787 [0]). This patch adds ConstantPointerNull to the list of constants that can be attached to DBG_VALUEs.

An open question is what to do about address-spaces and platforms where null is nonzero. Currently the code in SelectionDAGBuilder::getValueImpl just assumes that null is zero-valued, and I've read other parts of the code base that believe the default address space always has a zero valued null.  To be conservative, I've assigned null to be zero-valued for the default address space only, all the others get "undef" (which is an improvement in itself).

(More could be done on address spaces, but this patch accounts for the common case, and avoids having to ask any hard questions about how address space plumbing really works).

I haven't added an individual test for this behaviour: instead DebugInfo/X86/sdag-dangling-dbgvalue.ll needed updating to represent the new DBG_VALUEs being present. That test should trip if dbg.value of null ever goes away. Alternately I can add a dedicated test if that's desirable.

[0] https://bugs.llvm.org/show_bug.cgi?id=39787


Repository:
  rL LLVM

https://reviews.llvm.org/D55227

Files:
  lib/CodeGen/SelectionDAG/InstrEmitter.cpp
  lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  test/DebugInfo/X86/sdag-dangling-dbgvalue.ll


Index: test/DebugInfo/X86/sdag-dangling-dbgvalue.ll
===================================================================
--- test/DebugInfo/X86/sdag-dangling-dbgvalue.ll
+++ test/DebugInfo/X86/sdag-dangling-dbgvalue.ll
@@ -63,9 +63,9 @@
 @S = global %struct.SS { i32 23, i32 -17 }, align 4, !dbg !0
 
 ; Verify that the def comes before the for foo1.
-; TODO: Currently dbg.value for bar1 is dropped(?), is that expected?
 define i32 @test1() local_unnamed_addr #0 !dbg !17 {
 ; CHECK-LABEL: bb.0.entry1
+; CHECK-NEXT:    DBG_VALUE 0, $noreg, ![[BAR1]], !DIExpression()
 ; CHECK-NEXT:    [[REG1:%[0-9]+]]:gr64 =
 ; CHECK-NEXT:    DBG_VALUE [[REG1]], $noreg, ![[FOO1]], !DIExpression()
 entry1:
@@ -99,11 +99,9 @@
 }
 
 ; Verify that the def comes before the for bar4.
-; TODO: Currently dbg.value for foo4 is dropped. It is set to null and not
-;       used. Just like in test1 it can be discussed if there should be a
-;       DBG_VALUE for foo4 here.
 define i32 @test4() local_unnamed_addr #0 !dbg !40 {
 ; CHECK-LABEL: bb.0.entry4
+; CHECK-NEXT:    DBG_VALUE 0, $noreg, ![[FOO4]], !DIExpression()
 ; CHECK-NEXT:    [[REG4:%[0-9]+]]:gr64 =
 ; CHECK-NEXT:    DBG_VALUE [[REG4]], $noreg, ![[BAR4]], !DIExpression()
 entry4:
@@ -114,10 +112,9 @@
 }
 
 ; Verify that we do not get a DBG_VALUE that maps foo5 to @S here.
-; TODO: foo5 is set to null, and it is not really used. Just like in test1 it
-;       can be discussed if there should be a DBG_VALUE for foo5 here.
 define i32 @test5() local_unnamed_addr #0 !dbg !47 {
 ; CHECK-LABEL: bb.0.entry5:
+; CHECK-NEXT:    DBG_VALUE 0, $noreg, ![[FOO5]], !DIExpression()
 ; CHECK-NEXT:    [[REG5:%[0-9]+]]:gr64 =
 ; CHECK-NEXT:    DBG_VALUE [[REG5]], $noreg, ![[BAR5]], !DIExpression()
 ; CHECK-NOT:     DBG_VALUE [[REG5]], $noreg, ![[FOO5]], !DIExpression()
Index: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -5301,7 +5301,8 @@
       return nullptr;
 
     SDDbgValue *SDV;
-    if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V)) {
+    if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<UndefValue>(V) ||
+        isa<ConstantPointerNull>(V)) {
       SDV = DAG.getConstantDbgValue(Variable, Expression, V, dl, SDNodeOrder);
       DAG.AddDbgValue(SDV, nullptr, false);
       return nullptr;
Index: lib/CodeGen/SelectionDAG/InstrEmitter.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/InstrEmitter.cpp
+++ lib/CodeGen/SelectionDAG/InstrEmitter.cpp
@@ -735,6 +735,10 @@
         MIB.addImm(CI->getSExtValue());
     } else if (const ConstantFP *CF = dyn_cast<ConstantFP>(V)) {
       MIB.addFPImm(CF);
+    } else if (isa<ConstantPointerNull>(V) &&
+               V->getType()->getPointerAddressSpace() == 0) {
+      // For now, assume null pointers are zero-valued
+      MIB.addImm(0);
     } else {
       // Could be an Undef.  In any case insert an Undef so we can see what we
       // dropped.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55227.176425.patch
Type: text/x-patch
Size: 3160 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181203/89189c4e/attachment.bin>