Skip to content

Skip meta instructions for Builder.init with automatic location #73864

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

Merged
merged 2 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,34 @@ extension Type {
// Builder initialization
//===----------------------------------------------------------------------===//

private extension Instruction {
/// Returns self, unless self is a meta instruction, in which case the next
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a good definition of the term "meta instruction" - and also a better name. The term is not used anywhere else in SIL (I just figured out now that it's borrowed from LLVM's machine instructions).
In SILInstruction::isMetaInstruction() we define it as: "Every instruction that doesn't generate code should be in this list."
But this is not even true for alloc_stack: if the allocated type is generic, alloc_stack can generate code (metadata instantiation). On the other hand, there are a lot of instructions which really don't generate code, like dealloc_stack, begin_borrow, etc. and are not considered as "meta instructions".
So, what makes alloc_stack special in terms of debug info?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a good definition of the term "meta instruction" - and also a better name.

Yes we copied this term of art from MIR: https://www.llvm.org/doxygen/classllvm_1_1MachineInstr.html#aeffeb27bd92437aa2fd7b7567b01d078

Return true if this instruction doesn't produce any output in the form of executable instructions.

I think the definition we really mean in SIL is: Should a new SIL instruction inserted after this instruction inherit this instruction's SILLocation and scope?

Since alloc_stack instructions are often hoisted to the beginning of a function, they can have a scope and location that is out-of-order. You are correct that alloc_stack instruction generate code in the form of an LLVM alloca but these instructions are part of the function prologue and don't have any scope or location.

In modern SIL alloc_stack instructions can actually have two SIL location/scope pairs, one for the instruction, and one for the variable. We could side-step the problem by setting the location of the alloc_stack instruction to a compiler-generated (or prologue?) location and relying on the variable carrying its own scope and location. Then we don't need an IsMetaInstruction helper.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but that would still not solve the problem of what scope an alloc_stack should have. If we don't skip alloc_stacks in SILBuilderWithScope, then we'd need to reparent an alloc_stack every time it's moved. So I think we may still want to be able to skip them in SILBuilderWithScope.

So back to finding a better name for the function:
Since meta instruction is already a term of art in LLVM, I'd want to keep it.
The fact that dealloc_stack and begin_borrow are not classified as meta instructions is probably more a bug than a significant difference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing this in the doc comment:
"A meta instruction is an instruction whose location is not interesting as it is impossible to set a breakpoint on it. That could be because the instruction does not generate code (such as debug_value), or because the generated code would be in the prologue (alloc_stack). Other instructions should not inherit this instruction's location."

We could add other instructions to the list, but we would have to do it on both the C++ and the Swift side, so I think it should be in a separate PR.

/// non-meta instruction is returned. Returns nil if there are no non-meta
/// instructions in the basic block.
var nextNonMetaInstruction: Instruction? {
for inst in InstructionList(first: self) where !(inst is MetaInstruction) {
return inst
}
return nil
}

/// Returns the next interesting location. As it is impossible to set a
/// breakpoint on a meta instruction, those are skipped.
/// However, we don't want to take a location with different inlining
/// information than this instruction, so in that case, we will return the
/// location of the meta instruction. If the meta instruction is the only
/// instruction in the basic block, we also take its location.
var locationOfNextNonMetaInstruction: Location {
let location = self.location
guard !location.isInlined,
let nextLocation = nextNonMetaInstruction?.location,
!nextLocation.isInlined else {
return location
}
return nextLocation
}
}

extension Builder {
/// Creates a builder which inserts _before_ `insPnt`, using a custom `location`.
init(before insPnt: Instruction, location: Location, _ context: some MutatingContext) {
Expand All @@ -437,8 +465,23 @@ extension Builder {
context.notifyInstructionChanged, context._bridged.asNotificationHandler())
}

/// Creates a builder which inserts _before_ `insPnt`, using the location of `insPnt`.
/// Creates a builder which inserts before `insPnt`, using `insPnt`'s next
/// non-meta instruction's location.
/// This function should be used when moving code to an unknown insert point,
/// when we want to inherit the location of the closest non-meta instruction.
/// For replacing an existing meta instruction with another, use
/// ``Builder.init(replacing:_:)``.
init(before insPnt: Instruction, _ context: some MutatingContext) {
context.verifyIsTransforming(function: insPnt.parentFunction)
self.init(insertAt: .before(insPnt),
location: insPnt.locationOfNextNonMetaInstruction,
context.notifyInstructionChanged, context._bridged.asNotificationHandler())
}

/// Creates a builder which inserts _before_ `insPnt`, using the exact location of `insPnt`,
/// for the purpose of replacing that meta instruction with an equivalent instruction.
/// This function does not delete `insPnt`.
init(replacing insPnt: MetaInstruction, _ context: some MutatingContext) {
context.verifyIsTransforming(function: insPnt.parentFunction)
self.init(insertAt: .before(insPnt), location: insPnt.location,
context.notifyInstructionChanged, context._bridged.asNotificationHandler())
Expand All @@ -456,10 +499,10 @@ extension Builder {
}
}

/// Creates a builder which inserts _after_ `insPnt`, using the location of `insPnt`.
/// Creates a builder which inserts _after_ `insPnt`, using `insPnt`'s next
/// non-meta instruction's location.
init(after insPnt: Instruction, _ context: some MutatingContext) {
context.verifyIsTransforming(function: insPnt.parentFunction)
self.init(after: insPnt, location: insPnt.location, context)
self.init(after: insPnt, location: insPnt.locationOfNextNonMetaInstruction, context)
}

/// Creates a builder which inserts at the end of `block`, using a custom `location`.
Expand All @@ -478,11 +521,12 @@ extension Builder {
}

/// Creates a builder which inserts at the begin of `block`, using the location of the first
/// instruction of `block`.
/// non-meta instruction of `block`.
init(atBeginOf block: BasicBlock, _ context: some MutatingContext) {
context.verifyIsTransforming(function: block.parentFunction)
let firstInst = block.instructions.first!
self.init(insertAt: .before(firstInst), location: firstInst.location,
self.init(insertAt: .before(firstInst),
location: firstInst.locationOfNextNonMetaInstruction,
context.notifyInstructionChanged, context._bridged.asNotificationHandler())
}

Expand Down
13 changes: 11 additions & 2 deletions SwiftCompilerSources/Sources/SIL/Instruction.swift
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,16 @@ public protocol DebugVariableInstruction : VarDeclInstruction {
var debugVariable: DebugVariable { get }
}

final public class DebugValueInst : Instruction, UnaryInstruction, DebugVariableInstruction {
/// A meta instruction is an instruction whose location is not interesting as
/// it is impossible to set a breakpoint on it.
/// That could be because the instruction does not generate code (such as
/// `debug_value`), or because the generated code would be in the prologue
/// (`alloc_stack`).
/// When we are moving code onto an unknown instruction (such as the start of a
/// basic block), we want to ignore any meta instruction that might be there.
public protocol MetaInstruction: Instruction {}

final public class DebugValueInst : Instruction, UnaryInstruction, DebugVariableInstruction, MetaInstruction {
public var varDecl: VarDecl? {
VarDecl(bridged: bridged.DebugValue_getDecl())
}
Expand Down Expand Up @@ -1127,7 +1136,7 @@ final public class InitBlockStorageHeaderInst: SingleValueInstruction {}

public protocol Allocation : SingleValueInstruction { }

final public class AllocStackInst : SingleValueInstruction, Allocation, DebugVariableInstruction {
final public class AllocStackInst : SingleValueInstruction, Allocation, DebugVariableInstruction, MetaInstruction {
public var hasDynamicLifetime: Bool { bridged.AllocStackInst_hasDynamicLifetime() }

public var varDecl: VarDecl? {
Expand Down
1 change: 1 addition & 0 deletions SwiftCompilerSources/Sources/SIL/Location.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public struct Location: Equatable, CustomStringConvertible {

public var hasValidLineNumber: Bool { bridged.hasValidLineNumber() }
public var isAutoGenerated: Bool { bridged.isAutoGenerated() }
public var isInlined: Bool { bridged.isInlined() }

public var isDebugSteppable: Bool { hasValidLineNumber && !isAutoGenerated }

Expand Down
1 change: 1 addition & 0 deletions include/swift/SIL/SILBridging.h
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,7 @@ struct BridgedLocation {
SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedLocation getAutogeneratedLocation() const;
BRIDGED_INLINE bool hasValidLineNumber() const;
BRIDGED_INLINE bool isAutoGenerated() const;
BRIDGED_INLINE bool isInlined() const;
BRIDGED_INLINE bool isEqualTo(BridgedLocation rhs) const;
SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedSourceLoc getSourceLocation() const;
BRIDGED_INLINE bool hasSameSourceLocation(BridgedLocation rhs) const;
Expand Down
3 changes: 3 additions & 0 deletions include/swift/SIL/SILBridgingImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,9 @@ bool BridgedLocation::hasValidLineNumber() const {
bool BridgedLocation::isAutoGenerated() const {
return getLoc().isAutoGenerated();
}
bool BridgedLocation::isInlined() const {
return getLoc().getScope()->InlinedCallSite;
}
bool BridgedLocation::isEqualTo(BridgedLocation rhs) const {
return getLoc().isEqualTo(rhs.getLoc());
}
Expand Down
39 changes: 39 additions & 0 deletions test/DebugInfo/simplify_checked_cast_br.sil
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// RUN: %target-sil-opt -enable-sil-verify-all %s -simplification -simplify-instruction=checked_cast_br -sil-print-debuginfo | %FileCheck %s

// REQUIRES: swift_in_compiler

import Swift
import Builtin

protocol P : AnyObject {}
class B : P {}
class X : B {}

// CHECK-LABEL: sil [ossa] @test_ossa :
// CHECK: %0 = alloc_ref
// CHECK-NEXT: checked_cast_br AnyObject in %0 : $X
// CHECK: bb2([[A:%.*]] : @owned $X):
// CHECK-NEXT: [[U:%.*]] = upcast [[A]] : $X to $B, loc "takethis":1:1
// CHECK-NEXT: [[E:%.*]] = init_existential_ref [[U]] {{.+}}, loc "takethis":1:1
// CHECK-NEXT: debug_value [[E]] : $AnyObject, let, name "me", loc "ignore-this-meta-inst":1:1
// CHECK-NEXT: destroy_value [[E]] : $AnyObject, loc "takethis":1:1
// CHECK: } // end sil function 'test_ossa'
sil [ossa] @test_ossa : $@convention(thin) () -> @owned X {
bb0:
%0 = alloc_ref $X
%1 = upcast %0 : $X to $B
%2 = init_existential_ref %1 : $B : $B, $AnyObject
debug_value %2 : $AnyObject, name "x"
checked_cast_br AnyObject in %2 : $AnyObject to X, bb1, bb2

bb1(%5 : @owned $X):
return %5 : $X

bb2(%7 : @owned $AnyObject):
debug_value %7 : $AnyObject, let, name "me", loc "ignore-this-meta-inst":1:1
destroy_value %7: $AnyObject, loc "takethis":1:1
unreachable
}

sil_vtable X {
}