This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][SelectionDAG] Change order while transferring SDDbgValue to another node
ClosedPublic

Authored by krisb on Dec 8 2019, 5:41 AM.

Details

Summary

SelectionDAG::transferDbgValues() can 'reattach' SDDbgValue from one to another node, but doesn't change its source order. If the destination node has the order greater than the SDDbgValue, there are two possible issues revealed later:

  • If debug info is attached to an instruction that is the first definition of a register, this ends up with a def-after-use case and the debug info gets 'undef' later.
  • If MIR has another definition of a register above the debug info, debug info won't be 'undef' but may represent a source variable incorrectly because it appears (significantly) before an instruction corresponded to this debug info.

So, the patch changes the order of an SDDbgValue if it's being moved to a node with greater order.

Diff Detail

Event Timeline

krisb created this revision.Dec 8 2019, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2019, 5:41 AM
krisb edited the summary of this revision. (Show Details)Dec 8 2019, 5:43 AM
krisb added a project: debug-info.
krisb added a subscriber: avl.
bjope added a subscriber: bjope.Dec 8 2019, 12:37 PM

That seems reasonable.

llvm/test/DebugInfo/X86/sdag-transfer-dbgvalue.ll
28

Do we need all three dbg.values or could we further reduce the testcase by removing all but one?

djtodoro added inline comments.
llvm/test/DebugInfo/X86/sdag-transfer-dbgvalue.ll
74

I guess we do not need the TBAA metadata in the testcase.

This risks changing the order in which variable assignments appear to occur, as there's no consideration of whether the new IROrder goes past a dbg.value intrinsic of the same variable.

This patch is potentially worth paying that price though: AFAIUI, SelectionDAG::transferDbgValues happens all the time, but multiple debug intrinsics for the same variable in the same block aren't necessarily common. Running a clang-3.4 build with this patch, I see a few thousand more locations, which is great, although there's no headline-grabbing impact.

Note that ScheduleDAGSDNodes does attempt to emit DBG_VALUEs immediately after node definitions (ProcessSourceNode and nearby), but it's fairly flakey. I also wrote up some details in [0] about the fact that nothing considers both

  • where a value becomes available and
  • where the value should become the variable location.

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

krisb updated this revision to Diff 233378.Dec 11 2019, 8:46 AM
krisb marked 2 inline comments as done.
krisb edited the summary of this revision. (Show Details)

Reduced the test.

krisb added a comment.Dec 11 2019, 8:46 AM

@aprantl, @djtodoro thanks! I reduced the test a bit more.

@jmorse, thanks for pointing on this!

I found a couple of dozens of cases building clang where this patch causes SelectionDAG::transferDbgValues() to move processed debug value past another debug value of the same variable and I'm going to investigate them in details but it can take some time.

For now, I think we could conservatively let such debug values be on their places not changing their order (although it doesn't seem necessarily correct) and add corresponding TODO or FIXME note for further investigation.

It looks easy to check if a debug value gets moved past any others of the same variable: just to iterate over SDDbgValues and to find one that in-between From and To by their order. But there is one issue I faced that seems weird to me. I'd expect that a node should get an order that is greater than orders of its operands, but after DAGCombiner it's not true. I found lots of cases where a node has an order less than its operands. Do you know is it expected behavior?

aprantl accepted this revision.Dec 11 2019, 8:48 AM
aprantl added inline comments.
llvm/test/DebugInfo/X86/sdag-transfer-dbgvalue.ll
28

Another trick you can do is to delete all unneeded !dbg attachements or make them all point to the same location.

This revision is now accepted and ready to land.Dec 11 2019, 8:48 AM

I found a couple of dozens of cases building clang where this patch causes SelectionDAG::transferDbgValues() to move processed debug value past another debug value of the same variable and I'm going to investigate them in details but it can take some time.

For now, I think we could conservatively let such debug values be on their places not changing their order (although it doesn't seem necessarily correct) and add corresponding TODO or FIXME note for further investigation.

Yeah, the rest of the code around SelectionDAG doesn't do a good job of keeping location intrinsics in order; I think it's acceptable to suffer a little bit more re-ordering here for an increase in available locations, for now.

It looks easy to check if a debug value gets moved past any others of the same variable: just to iterate over SDDbgValues and to find one that in-between From and To by their order.

This is potentially expensive, particularly in ASAN builds where there are lots of variable locations. It might be worth benchmarking though in case I'm wrong.

But there is one issue I faced that seems weird to me. I'd expect that a node should get an order that is greater than orders of its operands, but after DAGCombiner it's not true. I found lots of cases where a node has an order less than its operands. Do you know is it expected behavior?

Ah, that's interesting -- according to comments for the "IROrder" field in SelectionDAGNodes.h, that number can be used to order node output instead of using a scheduler. So I wouldn't expect to see nodes with lesser order numbers. That being said, I'm not a SelectionDAG expert, so there could be some other explanation.

krisb updated this revision to Diff 235351.Dec 26 2019, 8:51 AM

Rebased and reduced the test a bit more.

krisb marked an inline comment as done.Dec 26 2019, 9:21 AM

Yeah, the rest of the code around SelectionDAG doesn't do a good job of keeping location intrinsics in order; I think it's acceptable to suffer a little bit more re-ordering here for an increase in available locations, for now.

Okay, I'll commit the patch as is, then. This issue with 'IROrder' in SelectionDAG is quite tricky, so I'll submit further changes in separate patches.

This revision was automatically updated to reflect the committed changes.