[semantic-arc] Move the Ownership Model Eliminator and management of SILFunction::hasQualifiedOwnership in front of SILOptions::EnableSILOwnership.

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,

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<bool> 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<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;
   }
 

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;

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

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() {

lib/SIL/SILVerifier.cpp

@@ -106,6 +106,10 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
   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 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
   }
 #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,23 +1123,23 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     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()),
               "load [copy] or load [take] can only be applied to non-trivial "
               "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<SILVerifier> {
   }
 
   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<SILVerifier> {
   }
 
   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,22 +1188,22 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     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()),
               "store [init] or store [assign] can only be applied to "
               "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<SILVerifier> {
   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) {

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();

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);

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 : $()

test/SIL/Serialization/ownership_qualified_memopts.sil

@@ -12,34 +12,26 @@ 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 : $()
 }
 
 // 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()