-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Retain more debug info in optimized builds #85244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
d1aece3 to
53b21e3
Compare
adrian-prantl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
SwiftCompilerSources/Sources/Optimizer/FunctionPasses/TempLValueElimination.swift
Show resolved
Hide resolved
SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift
Outdated
Show resolved
Hide resolved
3faa1e9 to
fb593f4
Compare
| } | ||
| } | ||
|
|
||
| first.salvageDebugInfo(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why this is needed.
The following replace calls erase(instruction:) which itself calls salvageDebugInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replace is for the second instruction, while this salvage is for the first instruction. It should only be necessary in optimized builds, so I'm changing this code. The test for this case is DebugInfo/simplify_struct_extract.sil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay
118f114 to
af515fd
Compare
| if !preserveDebugInfo { | ||
| first.salvageDebugInfo(self) | ||
| } | ||
| erase(instructionIncludingDebugUses: first) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eeckstein erase(instructionIncludingDebugUses:) calls erase(instructionIncludingAllUsers:), which seems to have a postcondition that the operands of the erased instruction have no users. I'm not sure if that's relevant here though, since replacement is informally assumed to be one of first's operands, and this function replaces uses of second with replacement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine
aidan-hall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one point of confusion
|
@swift-ci smoke test |
1 similar comment
|
@swift-ci smoke test |
Specifically, improved debug info retention in: * tryReplaceRedundantInstructionPair, * splitAggregateLoad, * TempLValueElimination, * Mem2Reg, * salvageDebugInfo (added case for unchecked_enum_data instructions), * ConstantFolding. The changes to Mem2Reg allow debug info to be retained in the case tested by self-nostorage.swift in -O builds, so we have just enabled -O in that file instead of writing a new test for it.
af515fd to
9240379
Compare
|
macOS & Windows passed, but Linux had a crash in my new |
|
@swift-ci smoke test linux |
|
@swift-ci smoke test macos |
Depends on #85237: Ignore b353bb3 in this PR.