Retain more debug info in SIL optimization passes #85957
Conversation
aidan-hall
force-pushed
the
fix-debug-info-again
branch
3 times, most recently
from
e797b25 to
1cb333a
Compare
|
@swift-ci test |
|
@swift-ci smoke test |
|
@swift-ci please smoke benchmark |
adrian-prantl
left a comment
adrian-prantl
left a comment
There was a problem hiding this comment.
by salvaging info in erase(instructionIncludingDebugUses:), rather than inserting many explicit salvageDebugInfo calls throughout the code to make the test cases pass.
Nice
atrick
left a comment
atrick
left a comment
There was a problem hiding this comment.
LGTM, except I would either drop the notifyNewInstruictions change or have salvageDebugInfo return a bool if it made changes
| /// new `debug_value` instruction before this instruction is deleted. | ||
| public func salvageDebugInfo(of instruction: Instruction) { | ||
| BridgedContext.salvageDebugInfo(instruction.bridged) | ||
| notifyInstructionsChanged() |
There was a problem hiding this comment.
We should only notify instructions changed when a new debug_value was actually inserted. We don't want to do this if there was no debug_info to salvage.
I think my TODO refers to CanonicalizeInstruction::notifyNewInstruction. The SwiftCompilerSources bridging API was added later by @eeckstein. You can probably just delete that TODO for two reasons
- I don't think Canonicalization calls back to SwiftCompilerSources and we're not adding new C++ canonicalization
- I can't imagine any analysis would actually need to know whether a new
debug_valueinstruction was created
More generally, do we care at all if salvageDebugInfo invalidates or notifies the context?
@eeckstein what do you think and what's the best strategy for notifying the context about new debug_value? I'm fine either ignoring the new instruction, or returning a bool if any change was made and letting the bridging API call notifyInstructionsChanged conditionally.
There was a problem hiding this comment.
I have removed the notifyInstructionsChanged() call from the bridged function (see below).
aidan-hall
force-pushed
the
fix-debug-info-again
branch
from
1cb333a to
6f8cc4a
Compare
|
@swift-ci test |
|
@swift-ci smoke test |
This reverts commit b1eb70b. The original PR (swiftlang#85244) was reverted (swiftlang#85836) due to rdar://165667449 This version fixes the aforementioned issue, and (potentially) improves overall debug info retention by salvaging info in erase(instructionIncludingDebugUses:), rather than inserting many explicit salvageDebugInfo calls throughout the code to make the test cases pass. Bridging: Make salvageDebugInfo a method of MutatingContext This feels more consistent than making it a method of Instruction. DebugInfo: Salvage from trivially dead instructions This handles the case we were checking in constantFoldBuiltin, so we do not need to salvage debug info there any more.
aidan-hall
force-pushed
the
fix-debug-info-again
branch
from
6f8cc4a to
1510673
Compare
|
Previous MacOS platform test failed due to a timeout caused by verbose logging in an LLDB test, which was no longer needed: |
|
@swift-ci smoke test |
|
@swift-ci test macos |
|
@swift-ci smoke test macos |
3 participants
The original PR (#85244) was reverted (#85836) due to rdar://165667449
This version fixes the aforementioned issue, and (potentially) improves overall debug info retention by salvaging info in
erase(instructionIncludingDebugUses:), rather than inserting many explicitsalvageDebugInfocalls throughout the code to make the test cases pass.