Merge pull request #5420 from gottesmm/enable_ownership_model_eliminator_by_default
[semantic-arc] Move the Ownership Model Eliminator and management of SILFunction::hasQualifiedOwnership in front of SILOptions::EnableSILOwnership.
diff --git a/include/swift/AST/DiagnosticsParse.def b/include/swift/AST/DiagnosticsParse.def
index ef2a51f..ebe7200 100644
--- a/include/swift/AST/DiagnosticsParse.def
+++ b/include/swift/AST/DiagnosticsParse.def
@@ -517,6 +517,8 @@
"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 1e24f08..6f7dfce 100644
--- a/include/swift/SIL/SILFunction.h
+++ b/include/swift/SIL/SILFunction.h
@@ -176,16 +176,12 @@
/// 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<bool> HasQualifiedOwnership;
+ bool HasQualifiedOwnership = true;
SILFunction(SILModule &module, SILLinkage linkage,
StringRef mangledName, CanSILFunctionType loweredType,
@@ -293,26 +289,15 @@
bool isZombie() const { return Zombie; }
/// Returns true if this function has qualified ownership instructions in it.
- Optional<bool> 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<bool> 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 b9d1480..117bcd0 100644
--- a/lib/Parse/ParseSIL.cpp
+++ b/lib/Parse/ParseSIL.cpp
@@ -3985,8 +3985,12 @@
// 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 ae1b529..694fdaf 100644
--- a/lib/SIL/InstructionUtils.cpp
+++ b/lib/SIL/InstructionUtils.cpp
@@ -269,16 +269,11 @@
"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 @@
// 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 0c7dc3f..b1ff6b1 100644
--- a/lib/SIL/SILFunction.cpp
+++ b/lib/SIL/SILFunction.cpp
@@ -85,7 +85,7 @@
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 @@
// 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 f3996de..4727151 100644
--- a/lib/SIL/SILVerifier.cpp
+++ b/lib/SIL/SILVerifier.cpp
@@ -106,6 +106,10 @@
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<void()> &extraContext = nullptr) {
if (condition) return;
@@ -128,11 +132,8 @@
}
#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 <class T> typename CanTypeWrapperTraits<T>::type
@@ -1122,14 +1123,14 @@
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 @@
"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 @@
}
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 @@
}
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 @@
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 @@
"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 @@
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 c7f3c73..03999df 100644
--- a/lib/SILOptimizer/PassManager/Passes.cpp
+++ b/lib/SILOptimizer/PassManager/Passes.cpp
@@ -86,12 +86,10 @@
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 5199e82..d14c95d 100644
--- a/lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp
+++ b/lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp
@@ -151,10 +151,6 @@
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 8993163..c3c85f8 100644
--- a/test/SIL/Parser/ownership_qualified_memopts.sil
+++ b/test/SIL/Parser/ownership_qualified_memopts.sil
@@ -5,18 +5,14 @@
// 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 @@
// 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 e1e123a..5e7f446 100644
--- a/test/SIL/Serialization/ownership_qualified_memopts.sil
+++ b/test/SIL/Serialization/ownership_qualified_memopts.sil
@@ -12,15 +12,11 @@
// 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 @@
// 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()
