[LoadableByAddress] Fix debug info holes when expanding alloc_stack. #15575
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcci
force-pushed
the
loadable-debuginfo
branch
from
862a9c6 to
64e8189
Compare
dcci
changed the title
|
@adrian-prantl now with the correct patch, after coffee. |
adrian-prantl
requested changes
Mar 28, 2018
lib/IRGen/LoadableByAddress.cpp
Outdated
| // that aren't lowered to anything real (e.g. debug_value). | ||
| static const SILDebugScope *findAllocStackDebugScope(SILBasicBlock &BB) { | ||
| auto It = BB.begin(); | ||
| while (It != BB.end()) { |
There was a problem hiding this comment.
This looks like it could be a range-based-for.
lib/IRGen/LoadableByAddress.cpp
Outdated
| // Find the correct debug scope for alloc stack. We want to give to the | ||
| // expanded sequence the correct debug scope so we skip over instructions | ||
| // that aren't lowered to anything real (e.g. debug_value). | ||
| static const SILDebugScope *findAllocStackDebugScope(SILBasicBlock &BB) { |
There was a problem hiding this comment.
I think this utility would make sense in SILBasicBlock.h or even as a member function of SILBasicBlock. It's going to be useful for other passes that have similar issues, too. Perhaps call it getScopeOfFirstNonMetaInstruction.
lib/IRGen/LoadableByAddress.cpp
Outdated
| void LoadableStorageAllocation::replaceLoadWithCopyAddr( | ||
| LoadInst *optimizableLoad) { | ||
| SILValue value = optimizableLoad->getOperand(); | ||
|
|
||
| SILBuilderWithScope allocBuilder(pass.F->begin()->begin()); | ||
| SILBuilder allocBuilder(pass.F->begin()->begin()); | ||
| allocBuilder.setCurrentDebugScope(findAllocStackDebugScope(*pass.F->begin())); |
There was a problem hiding this comment.
Wouldn't it be better to add a new constructor to SILBuilderWithScope that has this behavior?
/// Creates a new SILBuilder with an insertion point at the beginning of BB and the debug scope from the first non-metainstruction in the BB.
explicit SILBuilderWithScope(SILBasicBlock *BB) {}
lib/IRGen/LoadableByAddress.cpp
Outdated
| static const SILDebugScope *findAllocStackDebugScope(SILBasicBlock &BB) { | ||
| auto It = BB.begin(); | ||
| while (It != BB.end()) { | ||
| if (!isMaintenanceInst(&*It)) |
There was a problem hiding this comment.
Let's rename isMaintenanceInst to isMetaInstruction. This is the same name we are using in MIR.
dcci
force-pushed
the
loadable-debuginfo
branch
from
64e8189 to
0c43de8
Compare
|
@swift-ci please test |
|
@adrian-prantl addressed all your comments. Please take another look. |
|
Build failed |
|
Build failed |
adrian-prantl
requested changes
Mar 28, 2018
include/swift/SIL/DebugUtils.h
Outdated
| /// Returns true if the instruction \p Inst is a meta instruction which | ||
| /// is relevant for debug information and does not get lowered to a real | ||
| /// instruction. | ||
| inline bool isMetaInstruction(SILInstruction *Inst) { |
There was a problem hiding this comment.
Can you make this a method of SILInstruction?
I also believe that every SILInstruction that implements getVarInfo() should be included in this list (DebugValueAddr, AllocBox, ...) .
|
Sure! |
dcci
changed the title
<rdar://problem/36876376>
dcci
force-pushed
the
loadable-debuginfo
branch
from
0c43de8 to
dfaf133
Compare
|
Done. I think as a follow up I'm going to move |
|
@swift-ci please test and merge |
1 similar comment
|
@swift-ci please test and merge |
adrian-prantl
approved these changes
Mar 28, 2018
felipepiovezan
added a commit
to felipepiovezan/swift
that referenced
this pull request
Jan 12, 2024
This function is clearly returning the opposite of what its name says:
```
const SILDebugScope *SILBasicBlock::getScopeOfFirstNonMetaInstruction() {
for (auto &Inst : *this)
if (Inst.isMetaInstruction())
return Inst.getDebugScope();
return begin()->getDebugScope();
}
```
Looking at the PR history (sadly GH doesn't preserve old versions of the patch...)
swiftlang#15575
There was this snippet of code:
```
// Find the correct debug scope for alloc stack. We want to give to the
// expanded sequence the correct debug scope so we skip over instructions
// that aren't lowered to anything real (e.g. debug_value).
static const SILDebugScope *findAllocStackDebugScope(SILBasicBlock &BB) {
auto It = BB.begin();
while (It != BB.end()) {
if (!isMaintenanceInst(&*It))
```
We don't know what used to be after that line but, based on the comments, it
must have been a `return It->getDebugScope()`.
Adrian then asked the author to make this a different helper function
`getScopeOfFirstNonMetaInstruction`, and the subsequence force-push had the code
we see today. So maybe it was in this conversion that the author made the
mistake?
Fixing the implementation doesn't cause any tests to fail (sadly the original PR
did not add any SIL->SIL tests, which would have been ideal).
Catfish-Man
pushed a commit
to Catfish-Man/swift
that referenced
this pull request
Jan 19, 2024
This function is clearly returning the opposite of what its name says:
```
const SILDebugScope *SILBasicBlock::getScopeOfFirstNonMetaInstruction() {
for (auto &Inst : *this)
if (Inst.isMetaInstruction())
return Inst.getDebugScope();
return begin()->getDebugScope();
}
```
Looking at the PR history (sadly GH doesn't preserve old versions of the patch...)
swiftlang#15575
There was this snippet of code:
```
// Find the correct debug scope for alloc stack. We want to give to the
// expanded sequence the correct debug scope so we skip over instructions
// that aren't lowered to anything real (e.g. debug_value).
static const SILDebugScope *findAllocStackDebugScope(SILBasicBlock &BB) {
auto It = BB.begin();
while (It != BB.end()) {
if (!isMaintenanceInst(&*It))
```
We don't know what used to be after that line but, based on the comments, it
must have been a `return It->getDebugScope()`.
Adrian then asked the author to make this a different helper function
`getScopeOfFirstNonMetaInstruction`, and the subsequence force-push had the code
we see today. So maybe it was in this conversion that the author made the
mistake?
Fixing the implementation doesn't cause any tests to fail (sadly the original PR
did not add any SIL->SIL tests, which would have been ideal).
carlos4242
pushed a commit
to carlos4242/swift
that referenced
this pull request
May 31, 2024
This function is clearly returning the opposite of what its name says:
```
const SILDebugScope *SILBasicBlock::getScopeOfFirstNonMetaInstruction() {
for (auto &Inst : *this)
if (Inst.isMetaInstruction())
return Inst.getDebugScope();
return begin()->getDebugScope();
}
```
Looking at the PR history (sadly GH doesn't preserve old versions of the patch...)
swiftlang#15575
There was this snippet of code:
```
// Find the correct debug scope for alloc stack. We want to give to the
// expanded sequence the correct debug scope so we skip over instructions
// that aren't lowered to anything real (e.g. debug_value).
static const SILDebugScope *findAllocStackDebugScope(SILBasicBlock &BB) {
auto It = BB.begin();
while (It != BB.end()) {
if (!isMaintenanceInst(&*It))
```
We don't know what used to be after that line but, based on the comments, it
must have been a `return It->getDebugScope()`.
Adrian then asked the author to make this a different helper function
`getScopeOfFirstNonMetaInstruction`, and the subsequence force-push had the code
we see today. So maybe it was in this conversion that the author made the
mistake?
Fixing the implementation doesn't cause any tests to fail (sadly the original PR
did not add any SIL->SIL tests, which would have been ideal).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We should also skip debug info as they don't really are lowered
to anything real. Add an helper so that we can use it in passes
as well.
@adrian-prantl I'm dealing with a bot failure, but I wanted to put this online so you can take a look and let me know if this is fine. I'm going to add a test & run the tests.
This is the last failure we have before enabling the verifier for debug info, so my plan is to flip the switch today (crossing fingers).