[DebugInfo][DAG] Reduce SelectionDAGs reordering of variables referring to Arguments

[jmorse]

Summary

This patch restricts SelectionDAGs movement of DBG_VALUEs for function Arguments, to try and only apply to function parameter variables. Note that this similar but *unrelated* to PR40188. Crucial information, here "Argument" refers to LLVM IR Values that represent inputs to the function.

Right now, if one refers to a function Argument IR Value with a dbg.value, the DBG_VALUE will be hoisted to the top of the function by EmitFuncArgumentDbgValue -- because SelectionDAG wants to put Arguments at the start of the function as often as possible. This is problematic if you have a local variable that takes on the Argument value at a later stage in the function, because that DBG_VALUE will be hoisted, possibly re-ordering it. I've also found examples of locations going missing because they've been hoisted past complicated control flow and (LiveDebugValues I think?) can't track it -- the code for this is copied in the added test case. xyzzy gets inlined into bar, its arguments DBG_VALUE hoisted to the top of bar, after which the location gets lost passed the switch statement.

This patch improves matters by increasing the set of dbg.values that can associate with a vreg, which allows normal variables and inlined arguments to point at vregs for Argument values without having the EmitFuncArgumentDbgValue specialness applied to them. This limits the set of dbg.values that *must* dangle and be resolved by EmitFuncArgumentDbgValue to:

• dbg.values referring to function Arguments,
• that are also parameter variables, and
• where those parameters are parameters to the current function, not inlined ones.

This patch doesn't completely save all normal variables: EmitFuncArgumentDbgValue should reject non-param Variables. However adding code to do this seems to increase the number of locations dropped, and triggers a variety of test failures I haven't looked into yet. Thus this change is an incremental improvement.

I don't see much variable-location-coverage gain in a build of clang-3.4 (0.3% increase), however bytes-in-scope coverage increases by 2%.

DebugInfo/Sparc/subreg.ll needs a fix as a result of this change: the Argument to that function gets optimised out, because nothing uses it. With this patch, the normal-variable DBG_VALUE referring to it gets optimised out too. IMO this is desired behaviour: it's true to say that the normal variable it gets optimised out, and we only intend to preserve parameters. To keep the test passing I mark the variable to be a parameter to the function: it then gets special handling and is preserved in the output.

[bjope]

Nice! Me and @dstenb actually discussed this kind of problem earlier today. So quite fun (and surprising) to see a solution pop up in the inbox.

The test case can probably be simplified a little bit (see inline comments in the test case).

test/DebugInfo/X86/dbg-value-arg-movement.ll

1 ↗

(On Diff #184731)

I think it is wise to add -mtriple in the RUN line (I never remember if it really is needed, but I think it might cause problems otherwise) .
It does not hurt that much to have it in the RUN line (even if there is a "target triple" in the IR), just to avoid the risk that some buildbot would fail otherwise.

84 ↗

(On Diff #184731)

I can't really see that "lala" is used anywhere. It should be possible to remove this, right?

86 ↗

(On Diff #184731)

Just set directory to "". Unless you really want to show your userid here :-)

98 ↗

(On Diff #184731)

You do not need the TBAA metadata here.

One easy way to do cleanup some unused metadata could be to simply run

opt -S test/DebugInfo/X86/dbg-value-arg-movement.ll

and it will strip away unused metadata automatically.

It will however not catch the unused DIGlobalVariable "lala" (unless you manually remove !0 from the list in !5 first).

And you'd ofcourse need to manually add the RUN line, and other comments including the CHECK:s again after the cleanup.

[bjope]

In D57584#1380763, @bjope wrote:

Nice! Me and @dstenb actually discussed this kind of problem earlier today. So quite fun (and surprising) to see a solution pop up in the inbox.

Maybe I was a little bit too quick to say that this solved our problem. Our problem is related to SelectionDAGBuilder::EmitFuncArgumentDbgValue hoiting dbg.value instructions referring to arguments (even if we just use the value from an argument to describe another variable that is assigned later on in the function). We will file a bugzilla ticket for that.

[bjope]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5357 ↗

(On Diff #184731)

Capitalize local variable name.

[aprantl]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5357 ↗

(On Diff #184731)

Wouldn't this condition incorrectly also identify an inlined argument of a recursive function call as being an argument of the outermost non-inlined function? I think you may also need to check the inlinedAt of the dbg.value is null.

[jmorse]

Strip useless stuff from test, attempt to catch more inlined params.

[jmorse]

In D57584#1380763, @bjope wrote:

Nice! Me and @dstenb actually discussed this kind of problem earlier today. So quite fun (and surprising) to see a solution pop up in the inbox.

Glad to be of service :)

In D57584#1380829, @bjope wrote:

Maybe I was a little bit too quick to say that this solved our problem. Our problem is related to SelectionDAGBuilder::EmitFuncArgumentDbgValue hoiting dbg.value instructions referring to arguments (even if we just use the value from an argument to describe another variable that is assigned later on in the function). We will file a bugzilla ticket for that.

It may be that replicating the IsParamOfFunc logic in this change into resolveDanglingDebugInfo resolves that. (I haven't explored doing that yet).

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5357 ↗

(On Diff #184731)

Ah yes, that would break. I've added a test that calls DISubprogram->describes here, lifted from r317702, would that be sufficient?

[bjope]

In D57584#1382939, @jmorse wrote:

In D57584#1380829, @bjope wrote:

Maybe I was a little bit too quick to say that this solved our problem. Our problem is related to SelectionDAGBuilder::EmitFuncArgumentDbgValue hoiting dbg.value instructions referring to arguments (even if we just use the value from an argument to describe another variable that is assigned later on in the function). We will file a bugzilla ticket for that.

It may be that replicating the IsParamOfFunc logic in this change into resolveDanglingDebugInfo resolves that. (I haven't explored doing that yet).

It is more like that we should not do EmitFuncArgumentDbgValue based on that the value originates from a param (since it hoists the DBG_VALUE to the start of the function just because the value is a param, even if the DIVariable that is assigned the value isn't in the scope of the function start). That should only be done if the DIVariable also describes a parameter (and preferably only if it is the same parameter as the value is related to), and probably only for the first occurence of a dbg.value that describes that parameter. I've been experimenting a bit with that. I'll hopefully have something that I can show in a PR/review later today.

[aprantl]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5357 ↗

(On Diff #184731)

I don't think that works, because the debug info of an inlined recursive call will also describe the same function. My understanding is that we want this condition to apply to all dbg.values that describe parameters of the function but not inlined valrables? If that's the case, the missing condition is DbgValueInst->getDebugLocation()->getInlinedAt() == nullptr.

But perhaps I'm misunderstanding what we want here.

void @f(myObj*) !dbg !4 {
  // Non-inlined this.
  call @llvm.dbg.value(metadata %0, metadata !1, metadata !DIExpression()), !dbg !2
  // Inlined instance of this of recursive function call.
  call @llvm.dbg.value(metadata %0, metadata !1, metadata !DIExpression()), !dbg !3
...

!1 = !DILocalVariable(name "this", ...)
!2 = !DILocation(scope: !4, line: 42)
!3 = !DILocation(scope: !4, line: 42, inlinedAt: !5)

[jmorse]

Address comments (revise filtering of inlined parameters).

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

5357 ↗

(On Diff #184731)

Aha -- I'd misunderstood the original suggestion as meaning "inlined params of the recursive functions should be treated like the outermost function", whoops. Thanks for the clarity.

[bjope]

test/DebugInfo/Sparc/subreg.ll

29 ↗

(On Diff #185095)

I'm using a different solution in: https://reviews.llvm.org/D57702
Maybe we should probably do something about this test case in a separate commit. It is apparently easy to break.

[bjope]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

4944 ↗

(On Diff #185095)

Is this FIXME still relevant after https://reviews.llvm.org/rL353735 ? Maybe it can be removed after rebase?

test/DebugInfo/Sparc/subreg.ll

29 ↗

(On Diff #185095)

In the end I did not need to modify this test case in https://reviews.llvm.org/rL353735 .
You might wanna check again after rebase if this change still is needed.

Anyway, I think it is a little bit too brutal to make "a" appear as an argument in this test. The variable apparently wasn't an argument in the original program. So if the problem is that %b isn't live through the the function, then perhaps you can let fn1 return %b instead?

[jmorse]

Rebase onto Bjorns changes, remove now un-necessary test change.

[jmorse]

The filtering from r353735 happily makes the sparc test change un-necessary.

I also tested whether filtering parameters so that they can't take VReg SDDbgValues is necessary any more. Removing the if block around the relevant code still makes DebugInfo/X86/sdag-split-arg.ll change behaviour. This doesn't surprise me: how fast-isel interacts with SelectionDAG is a mystery to me.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

4944 ↗

(On Diff #185095)

Removed FIXME, r353735 does indeed add sufficient filtering,

[bjope]

In D57584#1395982, @jmorse wrote:

The filtering from r353735 happily makes the sparc test change un-necessary.

I also tested whether filtering parameters so that they can't take VReg SDDbgValues is necessary any more. Removing the if block around the relevant code still makes DebugInfo/X86/sdag-split-arg.ll change behaviour. This doesn't surprise me: how fast-isel interacts with SelectionDAG is a mystery to me.

I have little experience of fast-isel myself, so I agree that it is kind of a mystery. Ideally when making changes in this area we probably should add test cases for fast-isel and non fast-isel. And I think the various scheduling modes for the SelectionDAG also could impact the result sometimes, as well as endianess and calling convention. So one might need to write lots of test cases to cover all scenarios.

[bjope]

LGTM! But remember to update the "summary" in the commit msg before commit. It is a little bit out-dated at the moment (e.g. the info about sparc test fixup).

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

case Intrinsic::dbg_value: {
     if (N.getNode()) {
       if (EmitFuncArgumentDbgValue(V, Variable, Expression, dl, false, N))
         return nullptr;
       SDV = getDbgValue(N, Variable, Expression, dl, SDNodeOrder);
       DAG.AddDbgValue(SDV, N.getNode(), false);
       return nullptr;
     }
 
+    // Special rules apply for the first dbg.values of parameter variables in a
+    // function. Identify them by the fact they reference Argument Values, that
+    // they're parameters, and they are parameters of the current function. We
+    // need to let them dangle until they get an SDNode.
+    bool IsParamOfFunc = isa<Argument>(V) && Variable->isParameter() &&
+                         !DI.getDebugLoc()->getInlinedAt();
+    if (!IsParamOfFunc) {
       // The value is not used in this block yet (or it would have an SDNode).
       // We still want the value to appear for the user if possible -- if it has
       // an associated VReg, we can refer to that instead.
-    if (!isa<Argument>(V)) {
       auto VMI = FuncInfo.ValueMap.find(V);
       if (VMI != FuncInfo.ValueMap.end()) {
         unsigned Reg = VMI->second;
         // If this is a PHI node, it may be split up into several MI PHI nodes
         // (in FunctionLoweringInfo::set).
         RegsForValue RFV(V->getContext(), TLI, DAG.getDataLayout(), Reg,
                          V->getType(), None);
         if (RFV.occupiesMultipleRegs()) {

llvm/trunk/test/DebugInfo/X86/dbg-value-arg-movement.ll

+; RUN: llc -mtriple=x86_64-unknown-unknown -start-after=codegenprepare -stop-before=expand-isel-pseudos %s -o - | FileCheck %s
+
+; Test the movement of dbg.values of arguments. SelectionDAG tries to be
+; helpful and places DBG_VALUEs of Arguments at the start of functions.
+; Unfortunately, this doesn't necessarily make sense, as one can specify an
+; Argument IR Value as a variable location anywhere in the program.
+;
+; Distinguish cases where we want to hoist DBG_VALUEs, and those where we
+; don't, by whether the referred to variable is a parameter to the current
+; function. In the test below, 'xyzzy' is a parameter to an inlined function,
+; but should not be hoisted to the start of the function.
+;
+; Original test case, in which 'xyzzy' became unavailable because its DBG_VALUE
+; landed far from any uses, compiled "clang -O2 -g" with inlining,
+;
+;    int ext(void);
+;
+;    int
+;    foo(int xyzzy)
+;    {
+;      xyzzy = ext() * xyzzy;
+;      xyzzy += 1;
+;      ext();
+;      return xyzzy;
+;    }
+;
+;    int
+;    bar(int baz, int qux)
+;    {
+;      int fish;
+;      switch (qux) {
+;      case 12:        fish = 8;        break;
+;      case 848:       fish = 0;        break;
+;      case 99999:     fish = -1;       break;
+;      default:        fish = 12;
+;      }
+;      qux %= fish;
+;      qux += foo(baz);
+;      return qux;
+;    }
+
+; CHECK: [[BAZVAR:![0-9]+]] = !DILocalVariable(name: "baz",
+; CHECK: [[XYZVAR:![0-9]+]] = !DILocalVariable(name: "xyzzy",
+
+; Start of MIR function block,
+; CHECK-LABEL: body
+; Expect DBG_VALUE of physreg,
+; CHECK:       DBG_VALUE $edi, $noreg, [[BAZVAR]]
+; Expect DBG_VALUE of virtreg,
+; CHECK:       DBG_VALUE [[ARGREG:%[0-9]+]], $noreg, [[BAZVAR]]
+; Label for next block,
+; CHECK-LABEL: bb.1.next
+; Correctly place dbg.value in the 'next' block.
+; CHECK:       DBG_VALUE [[ARGREG]], $noreg, [[XYZVAR]]
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @ext()
+
+define dso_local i32 @bar(i32, i32) local_unnamed_addr !dbg !7 {
+  %3 = srem i32 %1, %0, !dbg !15
+  call void @llvm.dbg.value(metadata i32 %0, metadata !12, metadata !DIExpression()), !dbg !16
+  br label %next
+
+next:                                             ; preds = %2
+  call void @llvm.dbg.value(metadata i32 %0, metadata !17, metadata !DIExpression()), !dbg !22
+  %4 = tail call i32 @ext(), !dbg !24
+  %5 = mul nsw i32 %3, %4, !dbg !25
+  %6 = add i32 %5, %0, !dbg !25
+  ret i32 %6, !dbg !25
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !2, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: ".")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 8.0.0"}
+!7 = distinct !DISubprogram(name: "bar", scope: !1, file: !1, line: 19, type: !8, scopeLine: 20, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12}
+!12 = !DILocalVariable(name: "baz", arg: 1, scope: !7, file: !1, line: 19, type: !10)
+!15 = !DILocation(line: 35, column: 7, scope: !7)
+!16 = !DILocation(line: 19, column: 9, scope: !7)
+!17 = !DILocalVariable(name: "xyzzy", arg: 1, scope: !18, file: !1, line: 10, type: !10)
+!18 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 10, type: !19, scopeLine: 11, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !21)
+!19 = !DISubroutineType(types: !20)
+!20 = !{!10, !10}
+!21 = !{!17}
+!22 = !DILocation(line: 10, column: 9, scope: !18, inlinedAt: !23)
+!23 = distinct !DILocation(line: 36, column: 10, scope: !7)
+!24 = !DILocation(line: 12, column: 11, scope: !18, inlinedAt: !23)
+!25 = !DILocation(line: 37, column: 3, scope: !7)