This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Sort the SDDbgValue list before assuming it is in IR order
ClosedPublic

Authored by rnk on Sep 22 2017, 3:23 PM.

Details

Summary

This code iterates the 'Orders' vector in parallel with the DbgValue
list, emitting all DBG_VALUEs that occurred between the last IR order
insertion point and the next insertion point. This assumes the
SDDbgValue list is sorted in IR order, which it usually is. However, it
is not sorted when a node with a debug value is replaced with another
one. When this happens, TransferDbgValues is called, and the new value
is added to the end of the list.

The problem can be solved by stably sorting the list by IR order.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Sep 22 2017, 3:23 PM
rnk updated this revision to Diff 116418.Sep 22 2017, 3:37 PM
  • Don't overwrite existing test case
Ka-Ka added inline comments.Sep 24 2017, 1:10 AM
llvm/test/DebugInfo/X86/dbg-value-transfer-order.ll
27–39 ↗(On Diff #116418)

I had problem recreating the fault as this testcase passed both with or without your changes in ScheduleDAGSDNodes.cpp, but then I realized that the checks in these lines don't check anything as they are missing a ':' after the CHECK.

Harbormaster completed remote builds in B10553: Diff 116418.
aprantl accepted this revision.Sep 25 2017, 8:47 AM

Thanks! It might be good to separate the refactoring into an NFC commit.

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
755 ↗(On Diff #116418)

Orders.push_back({Order, &*std::prev(IP)});

860 ↗(On Diff #116418)

Add a comment why the stable_sort is necessary?

This revision is now accepted and ready to land.Sep 25 2017, 8:47 AM
rnk marked an inline comment as done.Sep 25 2017, 9:14 AM

Thanks!

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
860 ↗(On Diff #116418)

I don't have a testcase to demonstrate any issue, but it will ensure that clang has the same output on all platforms regardless of the (usually deterministic) instability in sort order. Anyways, commented.

llvm/test/DebugInfo/X86/dbg-value-transfer-order.ll
27–39 ↗(On Diff #116418)

Oops. I added the colons and confirmed that the test fails without the sort. Without the fix, the DBG_VALUE for bit_offset appears at the end of .LBB0_3.

This revision was automatically updated to reflect the committed changes.