[DebugInfo][X86] Describe call site values for zero-valued imms

[dstenb]

Summary

Add zero-materializing XORs to X86's describeLoadedValue() hook in order
to produce call site values.

I have had to change the defs logic in collectCallSiteParameters() a bit
to be able to describe the XORs. The XORs implicitly define $eflags,
which would cause them to never be considered, due to a guard condition
that I->getNumDefs() is one. I have changed that condition so that we
now only consider instructions where a forwarded register overlaps with
the instruction's single explicit define. We still need to collect the implicit
defines of other forwarded registers to remove them from the work list.
I'm not sure how to move towards supporting instructions with multiple
explicit defines, cases where forwarded register are implicitly defined,
and/or cases where an instruction produces values for multiple forwarded
registers. Perhaps the describeLoadedValue() hook should take a register
argument, and we then leave it up to the hook to describe the loaded
value in that register? I have not yet encountered a situation where
that would be necessary though.

[dstenb]

I have experimented a bit with making the describeLoadedValue() hook return machine operand objects rather than pointers, as that can simplify the code slightly.

The MachineOperand class only holds POD members, and on x86-64 it is 32 bytes large. That combined with copy elision means that the overhead of returning a machine operand object does not become very large.

I benchmarked that on a 8-thread i7-8650U machine with 32 GB RAM. The benchmark consisted of building a clang 8.0 binary configured with:

-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DLLVM_TARGETS_TO_BUILD=X86 \
-DLLVM_USE_SANITIZER=Address \
-DCMAKE_CXX_FLAGS="-Xclang -femit-debug-entry-values -stdlib=libc++"

The average wall clock time increased by 4 seconds (0.1%), from 62:05 to 62:09. Would that be an acceptable trade-off for slightly simpler code? If so, I can upload a patch for that, and rebase this patch on top of that.

[vsk]

Thanks for working on this!

Having ParamLoadedValue contain a MachineOperand object makes sense to me. This would stave off an explosion of ad hoc OneOperand, TwoOperand, etc forever-objects. And you've demonstrated that the overhead is negligible.

Looking forward, I also think it makes sense to change the describeLoadedValue API s.t. it's specific about which loaded value is interesting. That can wait until there's a motivating test case.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

601 ↗

(On Diff #218916)

registers

649 ↗

(On Diff #218916)

Consider renaming these vectors with 's/Defs/ForwardedDefs/g'. This would clarify that these defs are both a) attached to I and b) forwarded.

656 ↗

(On Diff #218916)

auto Reg : concat<unsigned>(ExplicitDefs, ImplicitDefs) ?

665 ↗

(On Diff #218916)

I think this is a reasonable limitation to enforce until we have a clearer picture of how to handle instructions with multiple forwarded defines.

665 ↗

(On Diff #218916)

Should this be: I->getNumExplicitDefs() != 1 || ExplicitDefs.empty ?

[djtodoro]

Thanks for this! As we have discussed before, this is desirable for sure.

Having ParamLoadedValue contain a MachineOperand object makes sense to me. This would stave off an explosion of ad hoc OneOperand, TwoOperand, etc forever-objects. And you've demonstrated that the overhead is negligible.

Looking forward, I also think it makes sense to change the describeLoadedValue API s.t. it's specific about which loaded value is interesting. That can wait until there's a motivating test case.

+1

[dstenb]

Rebase on top of pointer -> object refactoring.

[dstenb]

Address the rest of the comments.

[dstenb]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

649 ↗

(On Diff #218916)

I renamed them to the slightly shorter FwdRegDefs to avoid some awkward indentation due to >80 cols.

656 ↗

(On Diff #218916)

Neat!

665 ↗

(On Diff #218916)

Oh, yes. Thanks for catching that!

[NikolaPrica]

This looks good! I'm happy to see that implicit operands can be removed from the work-list this efficiently.

Having ParamLoadedValue contain a MachineOperand object makes sense to me. This would stave off an explosion of ad hoc OneOperand, TwoOperand, etc forever-objects. And you've demonstrated that the overhead is negligible.

Looking forward, I also think it makes sense to change the describeLoadedValue API s.t. it's specific about which loaded value is interesting. That can wait until there's a motivating test case.

+1

+1

[vsk]

Lgtm.

[dstenb]

Thanks for the reviews! I'll land these commits shortly.

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

static void collectCallSiteParameters(const MachineInstr *CallMI,
   // for their call site value desctipion, if the call is within the entry MBB.
   // The RegsForEntryValues maps a forwarding register into the register holding
   // the entry value.
   // TODO: Handle situations when call site parameter value can be described
   // as the entry value within basic blocks other then the first one.
   bool ShouldTryEmitEntryVals = MBB->getIterator() == MF->begin();
   DenseMap<unsigned, unsigned> RegsForEntryValues;
 
-  // If the MI is an instruction defining a parameter's forwarding register,
-  // add it into the Defs. If the MI clobbers more then one register, we use
-  // the Defs in order to remove all the registers from
-  // the ForwardedRegWorklist, since we do not support such situations now.
+  // If the MI is an instruction defining one or more parameters' forwarding
+  // registers, add those defines. We can currently only describe forwarded
+  // registers that are explicitly defined, but keep track of implicit defines
+  // also to remove those registers from the work list.
   auto getForwardingRegsDefinedByMI = [&](const MachineInstr &MI,
-                                         SmallVectorImpl<unsigned> &Defs) {
+                                          SmallVectorImpl<unsigned> &Explicit,
+                                          SmallVectorImpl<unsigned> &Implicit) {
     if (MI.isDebugInstr())
       return;
 
     for (const MachineOperand &MO : MI.operands()) {
       if (MO.isReg() && MO.isDef() &&
           Register::isPhysicalRegister(MO.getReg())) {
         for (auto FwdReg : ForwardedRegWorklist) {
           if (TRI->regsOverlap(FwdReg, MO.getReg())) {
-            Defs.push_back(FwdReg);
+            if (MO.isImplicit())
+              Implicit.push_back(FwdReg);
+            else
+              Explicit.push_back(FwdReg);
             break;
           }
         }
       }
     }
   };
 
   auto finishCallSiteParam = [&](DbgValueLoc DbgLocVal, unsigned Reg) {
for (; I != MBB->rend(); ++I) {
     // If the next instruction is a call we can not interpret parameter's
     // forwarding registers or we finished the interpretation of all parameters.
     if (I->isCall())
       return;
 
     if (ForwardedRegWorklist.empty())
       return;
 
-    SmallVector<unsigned, 4> Defs;
-    getForwardingRegsDefinedByMI(*I, Defs);
-    if (Defs.empty())
+    SmallVector<unsigned, 4> ExplicitFwdRegDefs;
+    SmallVector<unsigned, 4> ImplicitFwdRegDefs;
+    getForwardingRegsDefinedByMI(*I, ExplicitFwdRegDefs, ImplicitFwdRegDefs);
+    if (ExplicitFwdRegDefs.empty() && ImplicitFwdRegDefs.empty())
       continue;
 
     // If the MI clobbers more then one forwarding register we must remove
     // all of them from the working list.
-    for (auto Reg : Defs)
+    for (auto Reg : concat<unsigned>(ExplicitFwdRegDefs, ImplicitFwdRegDefs))
       ForwardedRegWorklist.erase(Reg);
-    if (I->getNumDefs() != 1)
+
+    // The describeLoadedValue() hook currently does not have any information
+    // about which register it should describe in case of multiple defines, so
+    // for now we only handle instructions where a forwarded register is (at
+    // least partially) defined by the instruction's single explicit define.
+    if (I->getNumExplicitDefs() != 1 || ExplicitFwdRegDefs.empty())
       continue;
-    unsigned Reg = Defs[0];
+    unsigned Reg = ExplicitFwdRegDefs[0];
 
     if (auto ParamValue = TII->describeLoadedValue(*I)) {
       if (ParamValue->first.isImm()) {
         unsigned Val = ParamValue->first.getImm();
         DbgValueLoc DbgLocVal(ParamValue->second, Val);
         finishCallSiteParam(DbgLocVal, Reg);
       } else if (ParamValue->first.isReg()) {
         Register RegLoc = ParamValue->first.getReg();

llvm/trunk/lib/Target/X86/X86InstrInfo.cpp

if (Op && Op->isReg() && Op->getReg() == Op2.getReg() && Coef > 0) {
       }
     }
 
     DIExpression::appendOffset(Ops, Offset);
     Expr = DIExpression::get(MI.getMF()->getFunction().getContext(), Ops);
 
     return ParamLoadedValue(*Op, Expr);;
   }
+  case X86::XOR32rr: {
+    if (MI.getOperand(1).getReg() == MI.getOperand(2).getReg())
+      return ParamLoadedValue(MachineOperand::CreateImm(0), Expr);
+    return None;
+  }
   default:
     return TargetInstrInfo::describeLoadedValue(MI);
   }
 }
 
 /// This is an architecture-specific helper function of reassociateOps.
 /// Set special operand attributes for new instructions after reassociation.
 void X86InstrInfo::setSpecialOperandAttr(MachineInstr &OldMI1,

llvm/trunk/test/DebugInfo/X86/dbgcall-site-zero-valued-imms.ll

+; RUN: llc -O3 -debug-entry-values -filetype=obj -o - %s | llvm-dwarfdump - | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Verify that we produce call site entries for the zero-valued parameters.
+;
+; Based on the following reproducer:
+;
+; #include <stdint.h>
+; extern void callee(uint8_t, uint16_t, uint32_t, void *);
+; int caller() {
+;   callee(0, 0, 0, (void *)0);
+;   return 1;
+; }
+
+; CHECK: DW_TAG_GNU_call_site_parameter
+; CHECK-NEXT: DW_AT_location  (DW_OP_reg2 RCX)
+; CHECK-NEXT: DW_AT_GNU_call_site_value (DW_OP_lit0)
+
+; CHECK: DW_TAG_GNU_call_site_parameter
+; CHECK-NEXT: DW_AT_location  (DW_OP_reg1 RDX)
+; CHECK-NEXT: DW_AT_GNU_call_site_value (DW_OP_lit0)
+
+; CHECK: DW_TAG_GNU_call_site_parameter
+; CHECK-NEXT: DW_AT_location  (DW_OP_reg4 RSI)
+; CHECK-NEXT: DW_AT_GNU_call_site_value (DW_OP_lit0)
+
+; CHECK: DW_TAG_GNU_call_site_parameter
+; CHECK-NEXT: DW_AT_location  (DW_OP_reg5 RDI)
+; CHECK-NEXT: DW_AT_GNU_call_site_value (DW_OP_lit0)
+
+; Function Attrs: nounwind uwtable
+define i32 @caller() #0 !dbg !15 {
+entry:
+  tail call void @callee(i8 zeroext 0, i16 zeroext 0, i32 0, i8* null), !dbg !19
+  ret i32 1, !dbg !20
+}
+
+declare !dbg !5 void @callee(i8 zeroext, i16 zeroext, i32, i8*)
+
+attributes #0 = { nounwind uwtable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!11, !12, !13}
+!llvm.ident = !{!14}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 10.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, nameTableKind: None)
+!1 = !DIFile(filename: "zero.c", directory: "/")
+!2 = !{}
+!3 = !{!4, !5}
+!4 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!5 = !DISubprogram(name: "callee", scope: !1, file: !1, line: 2, type: !6, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+!6 = !DISubroutineType(types: !7)
+!7 = !{null, !8, !9, !10, !4}
+!8 = !DIBasicType(name: "unsigned char", size: 8, encoding: DW_ATE_unsigned_char)
+!9 = !DIBasicType(name: "unsigned short", size: 16, encoding: DW_ATE_unsigned)
+!10 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!11 = !{i32 2, !"Dwarf Version", i32 4}
+!12 = !{i32 2, !"Debug Info Version", i32 3}
+!13 = !{i32 1, !"wchar_size", i32 4}
+!14 = !{!"clang version 10.0.0"}
+!15 = distinct !DISubprogram(name: "caller", scope: !1, file: !1, line: 3, type: !16, scopeLine: 3, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!18}
+!18 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!19 = !DILocation(line: 4, scope: !15)
+!20 = !DILocation(line: 5, scope: !15)