[DebugInfo] Ignore bitcasts when lowering stack arg dbg.values

[dstenb]

Summary

Look past bitcasts when looking for parameter debug values that are
described by frame-index loads in EmitFuncArgumentDbgValue().

In the attached test case we would be left with an undef DBG_VALUE
for the parameter without this patch.

A similar fix was done for parameters passed in registers in D13005.

This fixes PR40777.

[jmorse]

LGTM, although I'll wait a bit for more opinions

[aprantl]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5309

LGTM; is this the most general place where to do this, or could we do this higher up in the function and catch more cases?

[bjope]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5309

I actually asked for a test case when the argument was in a register (hitting the uses of N above) when discussing this with David offline. Just to see if we were missing out on doing the same thing higher up in the function.
The answer from David was that there already is a test case for that scenario. We already peek-through-bitcast in getUnderlyingArgReg above, and there is a test case triggering that scenario.

One could argue that we should use getUnderlyingArgReg here as well. However, that one is also dealing with AssertSext etc. We have no test cases verifying correctness for those non-BITCAST-peek-through's done by getUnderlyingArgReg (maybe it is impossible to get trigger such situations here). So it feels more safe to only focus on BITCAST here right now.

So, LGTM!

[dstenb]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5309

Sorry, when putting together this patch I was unable to trigger any {Assert[SE]ext, Truncate} cases for frame indices, but it seems that it's possible.

For example for the p4 parameter in the following test case (compiled using --target=arm -mbig-endian -O2 -g):

extern void bar(short);

void foo(int p0, int p1, int p2, int p3, short p4) {
  bar(p4);
}

for which we have:

t14: i16 = truncate t13                                                                                                                                                                                                                                                                     
  t13: i32 = AssertSext t11, ValueType:ch:i16                                                                                                                                                                                                                                               
    t11: i32,ch = load<(load 4 from %fixed-stack.4294967295)> t0, FrameIndex:i32<-1>, undef:i32                                                                                                                                                                                             
      t0: ch = EntryToken

As the target is big-endian, I think we need to add an offset of 2 bytes to the debug expression so that we point to the right part of the 4-byte stack slot.

Debug values for parameters that are smaller than the stack slot seems to be a bit broken for big-endian targets. If I remove the call to bar, so that p4 is left unused, we instead end up with the following node passed to EmitFuncArgumentDbgValue:

t11: i32,ch = load<(load 4 from %fixed-stack.4294967295)> t0, FrameIndex:i32<-1>, undef:i32                                                                                                                                                                                                 
  t0: ch = EntryToken

resulting in the following DBG_VALUE after isel:

DBG_VALUE %fixed-stack.0, 0, !"p4", !DIExpression(), debug-location !23; reduce.c:3:48 line no:3

ultimately leading to the following debug value in the output: @DEBUG_VALUE: foo:p4 <- [$sp+0]. I have not been able to verify this with a debugger yet, but unless I'm overlooking something I think the parameter will be printed using the wrong 2 bytes of the stack slot?

Would it be okay for me to deliver this look-past-bitcast patch as a first step (perhaps with a FIXME comment in the code), and then at least write a TR for the other issue?

[dstenb]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5309

I wrote https://bugs.llvm.org/show_bug.cgi?id=40973 for those issues now.

[dstenb]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5309

Would it be okay for me to deliver this look-past-bitcast patch as a first step (perhaps with a FIXME comment in the code), and then at least write a TR for the other issue?

I'll go ahead and deliver this patch before the end of the week unless there are any objections, as it should be a clear improvement, and the issue with the truncates will probably require some more invasive changes, and I don't know how to proceed with that as of now.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (Reg && TargetRegisterInfo::isVirtualRegister(Reg)) {
         Reg = PR;
     }
     if (Reg) {
       Op = MachineOperand::CreateReg(Reg, false);
       IsIndirect = IsDbgDeclare;
     }
   }
 
-  if (!Op && N.getNode())
+  if (!Op && N.getNode()) {
     // Check if frame index is available.
-    if (LoadSDNode *LNode = dyn_cast<LoadSDNode>(N.getNode()))
+    SDValue LCandidate = peekThroughBitcasts(N);
+    if (LoadSDNode *LNode = dyn_cast<LoadSDNode>(LCandidate.getNode()))
       if (FrameIndexSDNode *FINode =
           dyn_cast<FrameIndexSDNode>(LNode->getBasePtr().getNode()))
         Op = MachineOperand::CreateFI(FINode->getIndex());
+  }
 
   if (!Op) {
     // Check if ValueMap has reg number.
     DenseMap<const Value *, unsigned>::iterator VMI = FuncInfo.ValueMap.find(V);
     if (VMI != FuncInfo.ValueMap.end()) {
       const auto &TLI = DAG.getTargetLoweringInfo();
       RegsForValue RFV(V->getContext(), TLI, DAG.getDataLayout(), VMI->second,
                        V->getType(), getABIRegCopyCC(V));

test/DebugInfo/ARM/float-stack-arg.ll

+; RUN: llc -mtriple=armv4t-unknown-unknown -start-after=codegenprepare -stop-before=expand-isel-pseudos -o - %s | FileCheck %s
+
+; Verify that a stack-referencing DBG_VALUE is emitted for p5 at the start of
+; the function.
+;
+; Reproducer for PR40777.
+;
+; Based on the following C reproducer:
+;
+;   float fn1(int p1, int p2, int p3, int p4, float p5) {
+;    return p5;
+;   }
+;
+; that was compiled using -O1 -g -S -emit-llvm.
+;
+; Irrelevant metadata, e.g. information about %p[1-4], has been stripped.
+
+; CHECK: ![[P5:[0-9]*]] = !DILocalVariable(name: "p5"
+
+define arm_aapcscc float @fn1(i32 %p1, i32 %p2, i32 %p3, i32 %p4, float returned %p5) #0 !dbg !7 {
+; CHECK-LABEL: bb.0.entry:
+; CHECK-NEXT: DBG_VALUE %fixed-stack.0, 0, ![[P5]]
+entry:
+  call void @llvm.dbg.value(metadata float %p5, metadata !17, metadata !DIExpression()), !dbg !18
+  ret float %p5, !dbg !19
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { norecurse nounwind readnone }
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 9.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "float.c", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"min_enum_size", i32 4}
+!6 = !{!"clang version 9.0.0"}
+!7 = distinct !DISubprogram(name: "fn1", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !11, !11, !11, !11, !10}
+!10 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !{!17}
+!17 = !DILocalVariable(name: "p5", arg: 5, scope: !7, file: !1, line: 1, type: !10)
+!18 = !DILocation(line: 1, scope: !7)
+!19 = !DILocation(line: 2, scope: !7)