DebugInfo was incorrectly retained for sinks and post-register allocation hoists in the machine LICM pass. This diff is intended to address the problem.
Details
- Reviewers
vsk aprantl Orlando jmorse TWeaver StephenTozer - Group Reviewers
Restricted Project - Commits
- rGbd7ff5d94f0f: [DebugInfo] Correct debuginfo for post-ra hoist and sink in Machine LICM
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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...
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
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.
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.