Page MenuHomePhabricator

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

Authored by jmorse on Fri, Feb 1, 6:47 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

jmorse created this revision.Fri, Feb 1, 6:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Feb 1, 6:48 AM
bjope added a subscriber: dstenb.Fri, Feb 1, 7:29 AM

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 added a comment.Fri, Feb 1, 8:10 AM

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 added inline comments.Fri, Feb 1, 10:03 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5357 ↗(On Diff #184731)

Capitalize local variable name.

aprantl added inline comments.Fri, Feb 1, 5:02 PM
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 updated this revision to Diff 185019.Mon, Feb 4, 3:31 AM

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

jmorse marked 7 inline comments as done.Mon, Feb 4, 3:40 AM

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 :)

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 added a comment.Mon, Feb 4, 4:37 AM

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 added inline comments.Mon, Feb 4, 9:34 AM
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 updated this revision to Diff 185095.Mon, Feb 4, 10:57 AM
jmorse marked 3 inline comments as done.

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 added inline comments.Mon, Feb 4, 12:03 PM
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 added inline comments.Tue, Feb 12, 2:45 PM
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 updated this revision to Diff 186618.Wed, Feb 13, 3:06 AM

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

jmorse marked an inline comment as done.Wed, Feb 13, 3:11 AM

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 added a comment.Wed, Feb 13, 4:11 AM

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 accepted this revision.Wed, Feb 13, 4:17 AM

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).

This revision is now accepted and ready to land.Wed, Feb 13, 4:17 AM
This revision was automatically updated to reflect the committed changes.