Page MenuHomePhabricator

[DebugInfo] Correct debuginfo for post-ra hoist and sink in Machine LICM
ClosedPublic

Authored by chrisjackson on May 13 2020, 8:13 AM.

Details

Summary

DebugInfo was incorrectly retained for sinks and post-register allocation hoists in the machine LICM pass. This diff is intended to address the problem.

Diff Detail

Event Timeline

chrisjackson created this revision.May 13 2020, 8:13 AM

Can you explain why the sunk instruction's location is considered incorrect? I would have naively assumed that for profiling particularly, you would want to retain the original location to ensure that it is counted.

llvm/lib/CodeGen/MachineLICM.cpp
641

Can a DBG_VALUE ever be hoisted out of a loop? In that case this would delete the inline info from the intrinsic and attribute the inlined variable to the wrong function.

Can you explain why the sunk instruction's location is considered incorrect? I would have naively assumed that for profiling particularly, you would want to retain the original location to ensure that it is counted.

I should think profiling is mostly concerned about control flow, and a sampling profiler would infer control flow from the source location of the instruction. If you sink an instruction but retain the original source location, a hit would be tallied for the moved-from block not the moved-to block, but the moved-to block would be more appropriate. I might be misunderstanding the profiler's needs though, this is not an area I am super familiar with.

I feel like we should start a HowToUpdateDebugInfo.rst document where we document the expected behavior for various code transformations and what we expect the debug info to look like afterwards...

vsk added inline comments.May 13 2020, 11:32 AM
llvm/test/DebugInfo/MIR/X86/mlicm-sink.mir
3

I think you'll need to pass --match-full-lines to FileCheck, or append a {{$}} pattern to the 'CHECK: nuw' line, for this test to be tight.

  • Removed some excess debug information from post-ra test
  • Made sink Filecheck test more strict
vsk accepted this revision.EditedMay 15 2020, 5:50 PM

This lgtm. I think relying on location fallthrough is the best we can do here. @aprantl @chrisjackson I sketched a 'how to update debug info' doc in D80052: if that's any good, maybe we can iterate on it and codify the rationale/guidelines to follow for these sorts of changes.

This revision is now accepted and ready to land.May 15 2020, 5:50 PM
This revision was automatically updated to reflect the committed changes.
anil9 added a subscriber: anil9.May 26 2020, 5:52 PM
This comment was removed by anil9.