From 4413a169e4d79336ed7912a597aa6c094ac34517 Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Thu, 29 Feb 2024 13:22:15 +0100 Subject: [PATCH 1/3] SIL: fix: SILCloner doesn't create SILUndefs correctly and add verification for that cloned SILUndefs ended up with the wrong parent function --- include/swift/SIL/SILCloner.h | 4 +--- lib/SIL/Verifier/SILVerifier.cpp | 4 ++++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/swift/SIL/SILCloner.h b/include/swift/SIL/SILCloner.h index 44d483cd18fa1..c740c47c4aaab 100644 --- a/include/swift/SIL/SILCloner.h +++ b/include/swift/SIL/SILCloner.h @@ -592,9 +592,7 @@ SILCloner::getMappedValue(SILValue Value) { // If we have undef, just remap the type. if (auto *U = dyn_cast(Value)) { auto type = getOpType(U->getType()); - ValueBase *undef = - (type == U->getType() ? U : SILUndef::get(Builder.getFunction(), type)); - return SILValue(undef); + return SILUndef::get(Builder.getFunction(), type); } llvm_unreachable("Unmapped value while cloning?"); diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp index 7c2bc38775e62..8f073d521ef13 100644 --- a/lib/SIL/Verifier/SILVerifier.cpp +++ b/lib/SIL/Verifier/SILVerifier.cpp @@ -1263,6 +1263,10 @@ class SILVerifier : public SILVerifierBase { "instruction isn't dominated by its bb argument operand"); } + if (auto *undef = dyn_cast(operand.get())) { + require(undef->getParent() == BB->getParent(), "SILUndef in wrong function"); + } + require(operand.getUser() == I, "instruction's operand's owner isn't the instruction"); require(isOperandInValueUses(&operand), "operand value isn't used by operand"); From 58188fcee258bebf7802a8d7f032bb550cc72bcc Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Thu, 29 Feb 2024 13:43:39 +0100 Subject: [PATCH 2/3] SIL: let the function entry block be the parent block of SILUndefs in the function. --- lib/SIL/IR/SILValue.cpp | 4 ++++ test/SILOptimizer/simplify_cfg_args.sil | 8 ++++---- test/SILOptimizer/simplify_cfg_args_ossa.sil | 8 ++++---- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/SIL/IR/SILValue.cpp b/lib/SIL/IR/SILValue.cpp index 2b1abc7684c76..0cb1060c217fe 100644 --- a/lib/SIL/IR/SILValue.cpp +++ b/lib/SIL/IR/SILValue.cpp @@ -209,6 +209,10 @@ SILBasicBlock *SILNode::getParentBlock() const { if (auto *MVR = dyn_cast(this)) { return MVR->getParent()->getParent(); } + if (auto *undef = dyn_cast(this)) { + // By convention, undefs are considered to be defined at the entry of the function. + return undef->getParent()->getEntryBlock(); + } return nullptr; } diff --git a/test/SILOptimizer/simplify_cfg_args.sil b/test/SILOptimizer/simplify_cfg_args.sil index 9ed24cf772981..ad0ac9e96aee5 100644 --- a/test/SILOptimizer/simplify_cfg_args.sil +++ b/test/SILOptimizer/simplify_cfg_args.sil @@ -745,14 +745,14 @@ enum ThreeWay { // CHECK-LABEL: sil hidden [noinline] @testUnwrapEnumArg : $@convention(thin) () -> () { // CHECK: bb0: -// CHECK: br bb1(undef : $Builtin.Int64, %{{.*}} : $Payload) -// CHECK: bb1(%{{.*}} : $Builtin.Int64, %{{.*}} : $Payload): +// CHECK: br bb1(%{{.*}} : $Payload) +// CHECK: bb1(%{{.*}} : $Payload): // CHECK: alloc_stack $Payload // CHECK: switch_enum undef : $ThreeWay, case #ThreeWay.one!enumelt: bb4, case #ThreeWay.two!enumelt: bb2, case #ThreeWay.three!enumelt: bb3 // CHECK: bb2: // CHECK: [[P2:%.*]] = load %{{.*}} : $*Payload // CHECK: dealloc_stack %{{.*}} : $*Payload -// CHECK: br bb1(undef : $Builtin.Int64, [[P2]] : $Payload) +// CHECK: br bb1([[P2]] : $Payload) // CHECK: bb3: // CHECK: [[P3:%.*]] = load %{{.*}} : $*Payload // CHECK: dealloc_stack %{{.*}} : $*Payload @@ -764,7 +764,7 @@ enum ThreeWay { // CHECK: bb5: // CHECK: br bb7 // CHECK: bb6: -// CHECK: br bb1(undef : $Builtin.Int64, [[P4]] : $Payload) +// CHECK: br bb1([[P4]] : $Payload) // CHECK: bb7: // CHECK: return %{{.*}} : $() // CHECK-LABEL: } // end sil function 'testUnwrapEnumArg' diff --git a/test/SILOptimizer/simplify_cfg_args_ossa.sil b/test/SILOptimizer/simplify_cfg_args_ossa.sil index 40ca0a6f8b256..882dde55fc62e 100644 --- a/test/SILOptimizer/simplify_cfg_args_ossa.sil +++ b/test/SILOptimizer/simplify_cfg_args_ossa.sil @@ -723,14 +723,14 @@ enum ThreeWay { // CHECK-LABEL: sil hidden [noinline] [ossa] @testUnwrapEnumArg : $@convention(thin) () -> () { // CHECK: bb0: -// CHECK: br bb1(undef : $Builtin.Int64, %{{.*}} : $Payload) -// CHECK: bb1(%{{.*}} : $Builtin.Int64, %{{.*}} : $Payload): +// CHECK: br bb1(%{{.*}} : $Payload) +// CHECK: bb1(%{{.*}} : $Payload): // CHECK: alloc_stack $Payload // CHECK: switch_enum undef : $ThreeWay, case #ThreeWay.one!enumelt: bb4, case #ThreeWay.two!enumelt: bb2, case #ThreeWay.three!enumelt: bb3 // CHECK: bb2: // CHECK: [[P2:%.*]] = load [trivial] %{{.*}} : $*Payload // CHECK: dealloc_stack %{{.*}} : $*Payload -// CHECK: br bb1(undef : $Builtin.Int64, [[P2]] : $Payload) +// CHECK: br bb1([[P2]] : $Payload) // CHECK: bb3: // CHECK: [[P3:%.*]] = load [trivial] %{{.*}} : $*Payload // CHECK: dealloc_stack %{{.*}} : $*Payload @@ -740,7 +740,7 @@ enum ThreeWay { // CHECK: dealloc_stack %{{.*}} : $*Payload // CHECK: cond_br undef, bb6, bb5 // CHECK: bb5: -// CHECK: br bb1(undef : $Builtin.Int64, [[P4]] : $Payload) +// CHECK: br bb1([[P4]] : $Payload) // CHECK: bb6: // CHECK: br bb7 // CHECK: bb7: From 8137adf89e2b3685a3041859af890acd37414177 Mon Sep 17 00:00:00 2001 From: Erik Eckstein Date: Wed, 28 Feb 2024 11:33:21 +0100 Subject: [PATCH 3/3] SwiftCompilerSources: support `Undef.parentFunction` and `PlaceholderValue.parentFunction` After support in the C++ SIL, we can implement parentFunction for those values in swift, too. It simplifies a few things. --- .../Utilities/LifetimeDependenceUtils.swift | 2 +- .../Sources/Optimizer/Utilities/OptUtils.swift | 13 +++++-------- SwiftCompilerSources/Sources/SIL/Value.swift | 16 +++++++++++++--- include/swift/SIL/SILBridging.h | 2 ++ include/swift/SIL/SILBridgingImpl.h | 8 ++++++++ 5 files changed, 29 insertions(+), 12 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift b/SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift index c4307d624f85b..9dc0d12ffb682 100644 --- a/SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift +++ b/SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift @@ -185,7 +185,7 @@ struct LifetimeDependence : CustomStringConvertible { var parentValue: Value { scope.parentValue } var function: Function { - dependentValue.parentFunction // dependentValue can't be undef + dependentValue.parentFunction } var description: String { diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift b/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift index 65af91344f051..a8fdf25952d17 100644 --- a/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift +++ b/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift @@ -47,9 +47,6 @@ extension Value { /// check, because it treats a value_to_bridge_object instruction as "trivial". /// It can also handle non-trivial enums with trivial cases. func isTrivial(_ context: some Context) -> Bool { - if self is Undef { - return true - } var worklist = ValueWorklist(context) defer { worklist.deinitialize() } @@ -64,12 +61,12 @@ extension Value { switch v { case is ValueToBridgeObjectInst: break - case is StructInst, is TupleInst: - let inst = (v as! SingleValueInstruction) - worklist.pushIfNotVisited(contentsOf: inst.operands.values.filter { !($0 is Undef) }) + case let si as StructInst: + worklist.pushIfNotVisited(contentsOf: si.operands.values) + case let ti as TupleInst: + worklist.pushIfNotVisited(contentsOf: ti.operands.values) case let en as EnumInst: - if let payload = en.payload, - !(payload is Undef) { + if let payload = en.payload { worklist.pushIfNotVisited(payload) } default: diff --git a/SwiftCompilerSources/Sources/SIL/Value.swift b/SwiftCompilerSources/Sources/SIL/Value.swift index 4c4d561d30bbb..5ad69ca7f0408 100644 --- a/SwiftCompilerSources/Sources/SIL/Value.swift +++ b/SwiftCompilerSources/Sources/SIL/Value.swift @@ -25,10 +25,13 @@ public protocol Value : AnyObject, CustomStringConvertible { var definingInstruction: Instruction? { get } /// The block where the value is defined. - /// - /// It's not legal to get the definingBlock of an `Undef` value. var parentBlock: BasicBlock { get } + /// The function where the value lives in. + /// + /// It's not legal to get the parentFunction of an instruction in a global initializer. + var parentFunction: Function { get } + /// True if the value has a trivial type. var hasTrivialType: Bool { get } @@ -113,6 +116,7 @@ extension Value { public var uses: UseList { UseList(bridged.getFirstUse()) } + // Default implementation for all values which have a parent block, like instructions and arguments. public var parentFunction: Function { parentBlock.parentFunction } public var type: Type { bridged.getType().type } @@ -206,8 +210,11 @@ extension BridgedValue { public final class Undef : Value { public var definingInstruction: Instruction? { nil } + public var parentFunction: Function { bridged.SILUndef_getParentFunction().function } + public var parentBlock: BasicBlock { - fatalError("undef has no defining block") + // By convention, undefs are considered to be defined at the entry of the function. + parentFunction.entryBlock } /// Undef has not parent function, therefore the default `hasTrivialType` does not work. @@ -221,9 +228,12 @@ public final class Undef : Value { final class PlaceholderValue : Value { public var definingInstruction: Instruction? { nil } + public var parentBlock: BasicBlock { fatalError("PlaceholderValue has no defining block") } + + public var parentFunction: Function { bridged.PlaceholderValue_getParentFunction().function } } extension OptionalBridgedValue { diff --git a/include/swift/SIL/SILBridging.h b/include/swift/SIL/SILBridging.h index 4f7e892fa5277..0783fc7a0a981 100644 --- a/include/swift/SIL/SILBridging.h +++ b/include/swift/SIL/SILBridging.h @@ -414,6 +414,8 @@ struct BridgedValue { SWIFT_IMPORT_UNSAFE BRIDGED_INLINE OptionalBridgedOperand getFirstUse() const; SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedType getType() const; BRIDGED_INLINE Ownership getOwnership() const; + SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedFunction SILUndef_getParentFunction() const; + SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedFunction PlaceholderValue_getParentFunction() const; bool findPointerEscape() const; }; diff --git a/include/swift/SIL/SILBridgingImpl.h b/include/swift/SIL/SILBridgingImpl.h index 3e87a3cfe4710..cd9350b35366b 100644 --- a/include/swift/SIL/SILBridgingImpl.h +++ b/include/swift/SIL/SILBridgingImpl.h @@ -418,6 +418,14 @@ BridgedValue::Ownership BridgedValue::getOwnership() const { return castOwnership(getSILValue()->getOwnershipKind()); } +BridgedFunction BridgedValue::SILUndef_getParentFunction() const { + return {llvm::cast(getSILValue())->getParent()}; +} + +BridgedFunction BridgedValue::PlaceholderValue_getParentFunction() const { + return {llvm::cast(getSILValue())->getParent()}; +} + //===----------------------------------------------------------------------===// // BridgedOperand //===----------------------------------------------------------------------===//