From bb9197c7ff59a6bf7416bc1da4865ec5eb3f662b Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Sun, 23 Oct 2016 16:58:22 -0700 Subject: [PATCH] [semantic-arc] Move the Ownership Model Eliminator and management of SILFunction::hasQualifiedOwnership in front of SILOptions::EnableSILOwnership. This is a NFC change, since verification still will be behind the flag. But this will allow me to move copy_value, destroy_value in front of the EnableSILOwnership flag and verify via SILGen that we are always using those instructions. rdar://28851920 --- include/swift/AST/DiagnosticsParse.def | 2 + include/swift/SIL/SILFunction.h | 23 ++------ lib/Parse/ParseSIL.cpp | 8 ++- lib/SIL/InstructionUtils.cpp | 9 +-- lib/SIL/SILFunction.cpp | 8 +-- lib/SIL/SILVerifier.cpp | 57 ++++++++++--------- lib/SILOptimizer/PassManager/Passes.cpp | 10 ++-- .../Transforms/OwnershipModelEliminator.cpp | 4 -- .../Parser/ownership_qualified_memopts.sil | 8 --- .../ownership_qualified_memopts.sil | 8 --- 10 files changed, 49 insertions(+), 88 deletions(-) diff --git a/include/swift/AST/DiagnosticsParse.def b/include/swift/AST/DiagnosticsParse.def index ef2a51fcd0648..ebe7200ae322a 100644 --- a/include/swift/AST/DiagnosticsParse.def +++ b/include/swift/AST/DiagnosticsParse.def @@ -517,6 +517,8 @@ ERROR(sil_dbg_unknown_key,none, "unknown key '%0' in debug variable declaration", (StringRef)) ERROR(sil_objc_with_tail_elements,none, "alloc_ref [objc] cannot have tail allocated elements", ()) +ERROR(found_unqualified_instruction_in_qualified_function,none, + "found unqualified instruction in qualified function '%0'", (StringRef)) // SIL Basic Blocks ERROR(expected_sil_block_name,none, diff --git a/include/swift/SIL/SILFunction.h b/include/swift/SIL/SILFunction.h index 1e24f081ebdb6..6f7dfce47d993 100644 --- a/include/swift/SIL/SILFunction.h +++ b/include/swift/SIL/SILFunction.h @@ -176,16 +176,12 @@ class SILFunction /// method itself. In this case we need to create a vtable stub for it. bool Zombie = false; - /// True if SILOwnership is enabled for this function. It is always false when - /// the SILOption HasQualifiedOwnership is not set. When the SILOption - /// HasQualifiedOwnership /is/ set, this value is initialized to true. Once - /// the - /// OwnershipModelEliminator runs on the function, it is set to false. + /// True if SILOwnership is enabled for this function. /// /// This enables the verifier to easily prove that before the Ownership Model /// Eliminator runs on a function, we only see a non-semantic-arc world and /// after the pass runs, we only see a semantic-arc world. - Optional HasQualifiedOwnership; + bool HasQualifiedOwnership = true; SILFunction(SILModule &module, SILLinkage linkage, StringRef mangledName, CanSILFunctionType loweredType, @@ -293,26 +289,15 @@ class SILFunction bool isZombie() const { return Zombie; } /// Returns true if this function has qualified ownership instructions in it. - Optional hasQualifiedOwnership() const { - if (HasQualifiedOwnership.hasValue()) - return HasQualifiedOwnership.getValue(); - return None; - } + bool hasQualifiedOwnership() const { return HasQualifiedOwnership; } /// Returns true if this function has unqualified ownership instructions in /// it. - Optional hasUnqualifiedOwnership() const { - if (HasQualifiedOwnership.hasValue()) - return !HasQualifiedOwnership.getValue(); - return None; - } + bool hasUnqualifiedOwnership() const { return !HasQualifiedOwnership; } /// Sets the HasQualifiedOwnership flag to false. This signals to SIL that no /// ownership instructions should be in this function any more. void setUnqualifiedOwnership() { - assert( - HasQualifiedOwnership.hasValue() && - "Should never set unqualified ownership when SILOwnership is disabled"); HasQualifiedOwnership = false; } diff --git a/lib/Parse/ParseSIL.cpp b/lib/Parse/ParseSIL.cpp index b9d14808a7989..117bcd0addb9d 100644 --- a/lib/Parse/ParseSIL.cpp +++ b/lib/Parse/ParseSIL.cpp @@ -3985,8 +3985,12 @@ bool SILParser::parseSILBasicBlock(SILBuilder &B) { // Evaluate how the just parsed instruction effects this functions Ownership // Qualification. For more details, see the comment on the // FunctionOwnershipEvaluator class. - if (!OwnershipEvaluator.evaluate(&*BB->rbegin())) - return true; + SILInstruction *ParsedInst = &*BB->rbegin(); + if (!OwnershipEvaluator.evaluate(ParsedInst)) { + P.diagnose(ParsedInst->getLoc().getSourceLoc(), + diag::found_unqualified_instruction_in_qualified_function, + F->getName()); + } } while (isStartOfSILInstruction()); return false; diff --git a/lib/SIL/InstructionUtils.cpp b/lib/SIL/InstructionUtils.cpp index ae1b5297a1be4..694fdafb092f5 100644 --- a/lib/SIL/InstructionUtils.cpp +++ b/lib/SIL/InstructionUtils.cpp @@ -269,16 +269,11 @@ bool FunctionOwnershipEvaluator::evaluate(SILInstruction *I) { "does not belong to the instruction " "that we are evaluating"); - // If SIL ownership is not enabled in this module, just return true. There is - // no further work to do here. - if (!I->getModule().getOptions().EnableSILOwnership) - return true; - switch (OwnershipQualifiedKindVisitor().visit(I)) { case OwnershipQualifiedKind::Unqualified: { // If we already know that the function has unqualified ownership, just // return early. - if (!F.get()->hasQualifiedOwnership().getValue()) + if (!F.get()->hasQualifiedOwnership()) return true; // Ok, so we know at this point that we have qualified ownership. If we have @@ -298,7 +293,7 @@ bool FunctionOwnershipEvaluator::evaluate(SILInstruction *I) { // have unqualified ownership, then we know that we have already seen an // unqualified ownership instruction. This means the function has both // qualified and unqualified instructions. =><=. - if (!F.get()->hasQualifiedOwnership().getValue()) + if (!F.get()->hasQualifiedOwnership()) return false; // Ok, at this point we know that we are still qualified. Since functions diff --git a/lib/SIL/SILFunction.cpp b/lib/SIL/SILFunction.cpp index 0c7dc3f6e8884..b1ff6b10e317e 100644 --- a/lib/SIL/SILFunction.cpp +++ b/lib/SIL/SILFunction.cpp @@ -85,7 +85,7 @@ SILFunction::SILFunction(SILModule &Module, SILLinkage Linkage, StringRef Name, Bare(isBareSILFunction), Transparent(isTrans), Fragile(isFragile), Thunk(isThunk), ClassVisibility(classVisibility), GlobalInitFlag(false), InlineStrategy(inlineStrategy), Linkage(unsigned(Linkage)), - KeepAsPublic(false), EffectsKindAttr(E), HasQualifiedOwnership() { + KeepAsPublic(false), EffectsKindAttr(E) { if (InsertBefore) Module.functions.insert(SILModule::iterator(InsertBefore), this); else @@ -96,12 +96,6 @@ SILFunction::SILFunction(SILModule &Module, SILLinkage Linkage, StringRef Name, // Set our BB list to have this function as its parent. This enables us to // splice efficiently basic blocks in between functions. BlockList.Parent = this; - - // If SILOwnership is not enabled, HasQualifiedOwnership is None. If - // SILOwnership is enabled, we always initialize functions to have - // SILOwnership initially. - if (Module.getOptions().EnableSILOwnership) - HasQualifiedOwnership = true; } SILFunction::~SILFunction() { diff --git a/lib/SIL/SILVerifier.cpp b/lib/SIL/SILVerifier.cpp index f3996deb25568..472715103e25d 100644 --- a/lib/SIL/SILVerifier.cpp +++ b/lib/SIL/SILVerifier.cpp @@ -106,6 +106,10 @@ class SILVerifier : public SILVerifierBase { SILVerifier(const SILVerifier&) = delete; void operator=(const SILVerifier&) = delete; public: + bool isSILOwnershipEnabled() const { + return F.getModule().getOptions().EnableSILOwnership; + } + void _require(bool condition, const Twine &complaint, const std::function &extraContext = nullptr) { if (condition) return; @@ -128,11 +132,8 @@ class SILVerifier : public SILVerifierBase { } #define require(condition, complaint) \ _require(bool(condition), complaint ": " #condition) -#define requireTrueOrNone(condition, complaint) \ - _require(!condition.hasValue() || bool(condition.getValue()), \ - complaint ": " #condition) -#define requireFalseOrNone(condition, complaint) \ - _require(!condition.hasValue() || !bool(condition.getValue()), \ +#define requireTrueAndSILOwnership(verifier, condition, complaint) \ + _require(!verifier->isSILOwnershipEnabled() || bool(condition), \ complaint ": " #condition) template typename CanTypeWrapperTraits::type @@ -1122,14 +1123,14 @@ class SILVerifier : public SILVerifierBase { case LoadOwnershipQualifier::Unqualified: // We should not see loads with unqualified ownership when SILOwnership is // enabled. - requireFalseOrNone( - F.hasQualifiedOwnership(), + requireTrueAndSILOwnership( + this, F.hasUnqualifiedOwnership(), "Load with unqualified ownership in a qualified function"); break; case LoadOwnershipQualifier::Copy: case LoadOwnershipQualifier::Take: - requireTrueOrNone( - F.hasQualifiedOwnership(), + requireTrueAndSILOwnership( + this, F.hasQualifiedOwnership(), "Load with qualified ownership in an unqualified function"); // TODO: Could probably make this a bit stricter. require(!LI->getType().isTrivial(LI->getModule()), @@ -1137,8 +1138,8 @@ class SILVerifier : public SILVerifierBase { "types"); break; case LoadOwnershipQualifier::Trivial: - requireTrueOrNone( - F.hasQualifiedOwnership(), + requireTrueAndSILOwnership( + this, F.hasQualifiedOwnership(), "Load with qualified ownership in an unqualified function"); require(LI->getType().isTrivial(LI->getModule()), "A load with trivial ownership must load a trivial type"); @@ -1147,8 +1148,8 @@ class SILVerifier : public SILVerifierBase { } void checkLoadBorrowInst(LoadBorrowInst *LBI) { - requireTrueOrNone( - F.hasQualifiedOwnership(), + requireTrueAndSILOwnership( + this, F.hasQualifiedOwnership(), "Inst with qualified ownership in a function that is not qualified"); require(LBI->getType().isObject(), "Result of load must be an object"); require(LBI->getType().isLoadable(LBI->getModule()), @@ -1160,8 +1161,8 @@ class SILVerifier : public SILVerifierBase { } void checkEndBorrowInst(EndBorrowInst *EBI) { - requireTrueOrNone( - F.hasQualifiedOwnership(), + requireTrueAndSILOwnership( + this, F.hasQualifiedOwnership(), "Inst with qualified ownership in a function that is not qualified"); // We allow for end_borrow to express relationships in between addresses and // values, but we require that the types are the same ignoring value @@ -1187,13 +1188,13 @@ class SILVerifier : public SILVerifierBase { case StoreOwnershipQualifier::Unqualified: // We should not see loads with unqualified ownership when SILOwnership is // enabled. - requireFalseOrNone(F.hasQualifiedOwnership(), - "Invalid load with unqualified ownership"); + requireTrueAndSILOwnership(this, F.hasUnqualifiedOwnership(), + "Invalid load with unqualified ownership"); break; case StoreOwnershipQualifier::Init: case StoreOwnershipQualifier::Assign: - requireTrueOrNone( - F.hasQualifiedOwnership(), + requireTrueAndSILOwnership( + this, F.hasQualifiedOwnership(), "Inst with qualified ownership in a function that is not qualified"); // TODO: Could probably make this a bit stricter. require(!SI->getSrc()->getType().isTrivial(SI->getModule()), @@ -1201,8 +1202,8 @@ class SILVerifier : public SILVerifierBase { "non-trivial types"); break; case StoreOwnershipQualifier::Trivial: - requireTrueOrNone( - F.hasQualifiedOwnership(), + requireTrueAndSILOwnership( + this, F.hasQualifiedOwnership(), "Inst with qualified ownership in a function that is not qualified"); require(SI->getSrc()->getType().isTrivial(SI->getModule()), "A store with trivial ownership must store a trivial type"); @@ -1352,17 +1353,19 @@ class SILVerifier : public SILVerifierBase { void checkCopyValueInst(CopyValueInst *I) { require(I->getOperand()->getType().isObject(), "Source value should be an object value"); - requireTrueOrNone(F.hasQualifiedOwnership(), - "copy_value is only valid in functions with qualified " - "ownership"); + requireTrueAndSILOwnership( + this, F.hasQualifiedOwnership(), + "copy_value is only valid in functions with qualified " + "ownership"); } void checkDestroyValueInst(DestroyValueInst *I) { require(I->getOperand()->getType().isObject(), "Source value should be an object value"); - requireTrueOrNone(F.hasQualifiedOwnership(), - "destroy_value is only valid in functions with qualified " - "ownership"); + requireTrueAndSILOwnership( + this, F.hasQualifiedOwnership(), + "destroy_value is only valid in functions with qualified " + "ownership"); } void checkReleaseValueInst(ReleaseValueInst *I) { diff --git a/lib/SILOptimizer/PassManager/Passes.cpp b/lib/SILOptimizer/PassManager/Passes.cpp index c7f3c73cffe8e..03999dfdb3526 100644 --- a/lib/SILOptimizer/PassManager/Passes.cpp +++ b/lib/SILOptimizer/PassManager/Passes.cpp @@ -86,12 +86,10 @@ bool swift::runSILDiagnosticPasses(SILModule &Module) { return Ctx.hadError(); } - // If SILOwnership is enabled, run the ownership model eliminator. - if (Module.getOptions().EnableSILOwnership) { - PM.addOwnershipModelEliminator(); - PM.runOneIteration(); - PM.resetAndRemoveTransformations(); - } + // Lower all ownership instructions right after SILGen for now. + PM.addOwnershipModelEliminator(); + PM.runOneIteration(); + PM.resetAndRemoveTransformations(); // Otherwise run the rest of diagnostics. PM.addCapturePromotion(); diff --git a/lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp b/lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp index 5199e823d754c..d14c95d82f533 100644 --- a/lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp +++ b/lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp @@ -151,10 +151,6 @@ namespace { struct OwnershipModelEliminator : SILFunctionTransform { void run() override { SILFunction *F = getFunction(); - // We should only run this when SILOwnership is enabled. - assert(F->getModule().getOptions().EnableSILOwnership && - "Can not run ownership model eliminator when SIL ownership is not " - "enabled"); bool MadeChange = false; SILBuilder B(*F); OwnershipModelEliminatorVisitor Visitor(B); diff --git a/test/SIL/Parser/ownership_qualified_memopts.sil b/test/SIL/Parser/ownership_qualified_memopts.sil index 89931638e51a3..c3c85f8131541 100644 --- a/test/SIL/Parser/ownership_qualified_memopts.sil +++ b/test/SIL/Parser/ownership_qualified_memopts.sil @@ -5,18 +5,14 @@ import Builtin // CHECK-LABEL: sil @non_trivial : $@convention(thin) (@in Builtin.NativeObject, Builtin.NativeObject) -> () { // CHECK: bb0([[ARG1:%[0-9]+]] : $*Builtin.NativeObject, [[ARG2:%[0-9]+]] : $Builtin.NativeObject): -// CHECK: load [[ARG1]] : $*Builtin.NativeObject // CHECK: load [take] [[ARG1]] : $*Builtin.NativeObject // CHECK: load [copy] [[ARG1]] : $*Builtin.NativeObject -// CHECK: store [[ARG2]] to [[ARG1]] : $*Builtin.NativeObject // CHECK: store [[ARG2]] to [init] [[ARG1]] : $*Builtin.NativeObject // CHECK: store [[ARG2]] to [assign] [[ARG1]] : $*Builtin.NativeObject sil @non_trivial : $@convention(thin) (@in Builtin.NativeObject, Builtin.NativeObject) -> () { bb0(%0 : $*Builtin.NativeObject, %1 : $Builtin.NativeObject): - load %0 : $*Builtin.NativeObject load [take] %0 : $*Builtin.NativeObject load [copy] %0 : $*Builtin.NativeObject - store %1 to %0 : $*Builtin.NativeObject store %1 to [init] %0 : $*Builtin.NativeObject store %1 to [assign] %0 : $*Builtin.NativeObject %2 = tuple() @@ -25,15 +21,11 @@ bb0(%0 : $*Builtin.NativeObject, %1 : $Builtin.NativeObject): // CHECK-LABEL: sil @trivial : $@convention(thin) (@in Builtin.Int32, Builtin.Int32) -> () { // CHECK: bb0([[ARG1:%[0-9]+]] : $*Builtin.Int32, [[ARG2:%[0-9]+]] : $Builtin.Int32): -// CHECK: load [[ARG1]] : $*Builtin.Int32 // CHECK: load [trivial] [[ARG1]] : $*Builtin.Int32 -// CHECK: store [[ARG2]] to [[ARG1]] : $*Builtin.Int32 // CHECK: store [[ARG2]] to [trivial] [[ARG1]] : $*Builtin.Int32 sil @trivial : $@convention(thin) (@in Builtin.Int32, Builtin.Int32) -> () { bb0(%0 : $*Builtin.Int32, %1 : $Builtin.Int32): - load %0 : $*Builtin.Int32 load [trivial] %0 : $*Builtin.Int32 - store %1 to %0 : $*Builtin.Int32 store %1 to [trivial] %0 : $*Builtin.Int32 %2 = tuple() return %2 : $() diff --git a/test/SIL/Serialization/ownership_qualified_memopts.sil b/test/SIL/Serialization/ownership_qualified_memopts.sil index e1e123a19e50a..5e7f446044578 100644 --- a/test/SIL/Serialization/ownership_qualified_memopts.sil +++ b/test/SIL/Serialization/ownership_qualified_memopts.sil @@ -12,15 +12,11 @@ import Builtin // CHECK-LABEL: sil @trivial : $@convention(thin) (@in Builtin.Int32, Builtin.Int32) -> () { // CHECK: bb0([[ARG1:%[0-9]+]] : $*Builtin.Int32, [[ARG2:%[0-9]+]] : $Builtin.Int32): -// CHECK: load [[ARG1]] : $*Builtin.Int32 // CHECK: load [trivial] [[ARG1]] : $*Builtin.Int32 -// CHECK: store [[ARG2]] to [[ARG1]] : $*Builtin.Int32 // CHECK: store [[ARG2]] to [trivial] [[ARG1]] : $*Builtin.Int32 sil @trivial : $@convention(thin) (@in Builtin.Int32, Builtin.Int32) -> () { bb0(%0 : $*Builtin.Int32, %1 : $Builtin.Int32): - load %0 : $*Builtin.Int32 load [trivial] %0 : $*Builtin.Int32 - store %1 to %0 : $*Builtin.Int32 store %1 to [trivial] %0 : $*Builtin.Int32 %2 = tuple() return %2 : $() @@ -28,18 +24,14 @@ bb0(%0 : $*Builtin.Int32, %1 : $Builtin.Int32): // CHECK-LABEL: sil @non_trivial : $@convention(thin) (@in Builtin.NativeObject, Builtin.NativeObject) -> () { // CHECK: bb0([[ARG1:%[0-9]+]] : $*Builtin.NativeObject, [[ARG2:%[0-9]+]] : $Builtin.NativeObject): -// CHECK: load [[ARG1]] : $*Builtin.NativeObject // CHECK: load [take] [[ARG1]] : $*Builtin.NativeObject // CHECK: load [copy] [[ARG1]] : $*Builtin.NativeObject -// CHECK: store [[ARG2]] to [[ARG1]] : $*Builtin.NativeObject // CHECK: store [[ARG2]] to [init] [[ARG1]] : $*Builtin.NativeObject // CHECK: store [[ARG2]] to [assign] [[ARG1]] : $*Builtin.NativeObject sil @non_trivial : $@convention(thin) (@in Builtin.NativeObject, Builtin.NativeObject) -> () { bb0(%0 : $*Builtin.NativeObject, %1 : $Builtin.NativeObject): - load %0 : $*Builtin.NativeObject load [take] %0 : $*Builtin.NativeObject load [copy] %0 : $*Builtin.NativeObject - store %1 to %0 : $*Builtin.NativeObject store %1 to [init] %0 : $*Builtin.NativeObject store %1 to [assign] %0 : $*Builtin.NativeObject %2 = tuple()