Skip to content

Commit 68f53dc

Browse files
gottesmmgottesmm
authored
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.
2 parents 6ebb627 + bb9197c commit 68f53dc

File tree

10 files changed

+49
-88
lines changed

10 files changed

+49
-88
lines changed

‎include/swift/AST/DiagnosticsParse.def

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,8 @@ ERROR(sil_dbg_unknown_key,none,
517517
"unknown key '%0' in debug variable declaration", (StringRef))
518518
ERROR(sil_objc_with_tail_elements,none,
519519
"alloc_ref [objc] cannot have tail allocated elements", ())
520+
ERROR(found_unqualified_instruction_in_qualified_function,none,
521+
"found unqualified instruction in qualified function '%0'", (StringRef))
520522

521523
// SIL Basic Blocks
522524
ERROR(expected_sil_block_name,none,

‎include/swift/SIL/SILFunction.h

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -176,16 +176,12 @@ class SILFunction
176176
/// method itself. In this case we need to create a vtable stub for it.
177177
bool Zombie = false;
178178

179-
/// True if SILOwnership is enabled for this function. It is always false when
180-
/// the SILOption HasQualifiedOwnership is not set. When the SILOption
181-
/// HasQualifiedOwnership /is/ set, this value is initialized to true. Once
182-
/// the
183-
/// OwnershipModelEliminator runs on the function, it is set to false.
179+
/// True if SILOwnership is enabled for this function.
184180
///
185181
/// This enables the verifier to easily prove that before the Ownership Model
186182
/// Eliminator runs on a function, we only see a non-semantic-arc world and
187183
/// after the pass runs, we only see a semantic-arc world.
188-
Optional<bool> HasQualifiedOwnership;
184+
bool HasQualifiedOwnership = true;
189185

190186
SILFunction(SILModule &module, SILLinkage linkage,
191187
StringRef mangledName, CanSILFunctionType loweredType,
@@ -293,26 +289,15 @@ class SILFunction
293289
bool isZombie() const { return Zombie; }
294290

295291
/// Returns true if this function has qualified ownership instructions in it.
296-
Optional<bool> hasQualifiedOwnership() const {
297-
if (HasQualifiedOwnership.hasValue())
298-
return HasQualifiedOwnership.getValue();
299-
return None;
300-
}
292+
bool hasQualifiedOwnership() const { return HasQualifiedOwnership; }
301293

302294
/// Returns true if this function has unqualified ownership instructions in
303295
/// it.
304-
Optional<bool> hasUnqualifiedOwnership() const {
305-
if (HasQualifiedOwnership.hasValue())
306-
return !HasQualifiedOwnership.getValue();
307-
return None;
308-
}
296+
bool hasUnqualifiedOwnership() const { return !HasQualifiedOwnership; }
309297

310298
/// Sets the HasQualifiedOwnership flag to false. This signals to SIL that no
311299
/// ownership instructions should be in this function any more.
312300
void setUnqualifiedOwnership() {
313-
assert(
314-
HasQualifiedOwnership.hasValue() &&
315-
"Should never set unqualified ownership when SILOwnership is disabled");
316301
HasQualifiedOwnership = false;
317302
}
318303

‎lib/Parse/ParseSIL.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3985,8 +3985,12 @@ bool SILParser::parseSILBasicBlock(SILBuilder &B) {
39853985
// Evaluate how the just parsed instruction effects this functions Ownership
39863986
// Qualification. For more details, see the comment on the
39873987
// FunctionOwnershipEvaluator class.
3988-
if (!OwnershipEvaluator.evaluate(&*BB->rbegin()))
3989-
return true;
3988+
SILInstruction *ParsedInst = &*BB->rbegin();
3989+
if (!OwnershipEvaluator.evaluate(ParsedInst)) {
3990+
P.diagnose(ParsedInst->getLoc().getSourceLoc(),
3991+
diag::found_unqualified_instruction_in_qualified_function,
3992+
F->getName());
3993+
}
39903994
} while (isStartOfSILInstruction());
39913995

39923996
return false;

‎lib/SIL/InstructionUtils.cpp

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -269,16 +269,11 @@ bool FunctionOwnershipEvaluator::evaluate(SILInstruction *I) {
269269
"does not belong to the instruction "
270270
"that we are evaluating");
271271

272-
// If SIL ownership is not enabled in this module, just return true. There is
273-
// no further work to do here.
274-
if (!I->getModule().getOptions().EnableSILOwnership)
275-
return true;
276-
277272
switch (OwnershipQualifiedKindVisitor().visit(I)) {
278273
case OwnershipQualifiedKind::Unqualified: {
279274
// If we already know that the function has unqualified ownership, just
280275
// return early.
281-
if (!F.get()->hasQualifiedOwnership().getValue())
276+
if (!F.get()->hasQualifiedOwnership())
282277
return true;
283278

284279
// Ok, so we know at this point that we have qualified ownership. If we have
@@ -298,7 +293,7 @@ bool FunctionOwnershipEvaluator::evaluate(SILInstruction *I) {
298293
// have unqualified ownership, then we know that we have already seen an
299294
// unqualified ownership instruction. This means the function has both
300295
// qualified and unqualified instructions. =><=.
301-
if (!F.get()->hasQualifiedOwnership().getValue())
296+
if (!F.get()->hasQualifiedOwnership())
302297
return false;
303298

304299
// Ok, at this point we know that we are still qualified. Since functions

‎lib/SIL/SILFunction.cpp

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ SILFunction::SILFunction(SILModule &Module, SILLinkage Linkage, StringRef Name,
8585
Bare(isBareSILFunction), Transparent(isTrans), Fragile(isFragile),
8686
Thunk(isThunk), ClassVisibility(classVisibility), GlobalInitFlag(false),
8787
InlineStrategy(inlineStrategy), Linkage(unsigned(Linkage)),
88-
KeepAsPublic(false), EffectsKindAttr(E), HasQualifiedOwnership() {
88+
KeepAsPublic(false), EffectsKindAttr(E) {
8989
if (InsertBefore)
9090
Module.functions.insert(SILModule::iterator(InsertBefore), this);
9191
else
@@ -96,12 +96,6 @@ SILFunction::SILFunction(SILModule &Module, SILLinkage Linkage, StringRef Name,
9696
// Set our BB list to have this function as its parent. This enables us to
9797
// splice efficiently basic blocks in between functions.
9898
BlockList.Parent = this;
99-
100-
// If SILOwnership is not enabled, HasQualifiedOwnership is None. If
101-
// SILOwnership is enabled, we always initialize functions to have
102-
// SILOwnership initially.
103-
if (Module.getOptions().EnableSILOwnership)
104-
HasQualifiedOwnership = true;
10599
}
106100

107101
SILFunction::~SILFunction() {

‎lib/SIL/SILVerifier.cpp

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,10 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
106106
SILVerifier(const SILVerifier&) = delete;
107107
void operator=(const SILVerifier&) = delete;
108108
public:
109+
bool isSILOwnershipEnabled() const {
110+
return F.getModule().getOptions().EnableSILOwnership;
111+
}
112+
109113
void _require(bool condition, const Twine &complaint,
110114
const std::function<void()> &extraContext = nullptr) {
111115
if (condition) return;
@@ -128,11 +132,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
128132
}
129133
#define require(condition, complaint) \
130134
_require(bool(condition), complaint ": " #condition)
131-
#define requireTrueOrNone(condition, complaint) \
132-
_require(!condition.hasValue() || bool(condition.getValue()), \
133-
complaint ": " #condition)
134-
#define requireFalseOrNone(condition, complaint) \
135-
_require(!condition.hasValue() || !bool(condition.getValue()), \
135+
#define requireTrueAndSILOwnership(verifier, condition, complaint) \
136+
_require(!verifier->isSILOwnershipEnabled() || bool(condition), \
136137
complaint ": " #condition)
137138

138139
template <class T> typename CanTypeWrapperTraits<T>::type
@@ -1122,23 +1123,23 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
11221123
case LoadOwnershipQualifier::Unqualified:
11231124
// We should not see loads with unqualified ownership when SILOwnership is
11241125
// enabled.
1125-
requireFalseOrNone(
1126-
F.hasQualifiedOwnership(),
1126+
requireTrueAndSILOwnership(
1127+
this, F.hasUnqualifiedOwnership(),
11271128
"Load with unqualified ownership in a qualified function");
11281129
break;
11291130
case LoadOwnershipQualifier::Copy:
11301131
case LoadOwnershipQualifier::Take:
1131-
requireTrueOrNone(
1132-
F.hasQualifiedOwnership(),
1132+
requireTrueAndSILOwnership(
1133+
this, F.hasQualifiedOwnership(),
11331134
"Load with qualified ownership in an unqualified function");
11341135
// TODO: Could probably make this a bit stricter.
11351136
require(!LI->getType().isTrivial(LI->getModule()),
11361137
"load [copy] or load [take] can only be applied to non-trivial "
11371138
"types");
11381139
break;
11391140
case LoadOwnershipQualifier::Trivial:
1140-
requireTrueOrNone(
1141-
F.hasQualifiedOwnership(),
1141+
requireTrueAndSILOwnership(
1142+
this, F.hasQualifiedOwnership(),
11421143
"Load with qualified ownership in an unqualified function");
11431144
require(LI->getType().isTrivial(LI->getModule()),
11441145
"A load with trivial ownership must load a trivial type");
@@ -1147,8 +1148,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
11471148
}
11481149

11491150
void checkLoadBorrowInst(LoadBorrowInst *LBI) {
1150-
requireTrueOrNone(
1151-
F.hasQualifiedOwnership(),
1151+
requireTrueAndSILOwnership(
1152+
this, F.hasQualifiedOwnership(),
11521153
"Inst with qualified ownership in a function that is not qualified");
11531154
require(LBI->getType().isObject(), "Result of load must be an object");
11541155
require(LBI->getType().isLoadable(LBI->getModule()),
@@ -1160,8 +1161,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
11601161
}
11611162

11621163
void checkEndBorrowInst(EndBorrowInst *EBI) {
1163-
requireTrueOrNone(
1164-
F.hasQualifiedOwnership(),
1164+
requireTrueAndSILOwnership(
1165+
this, F.hasQualifiedOwnership(),
11651166
"Inst with qualified ownership in a function that is not qualified");
11661167
// We allow for end_borrow to express relationships in between addresses and
11671168
// values, but we require that the types are the same ignoring value
@@ -1187,22 +1188,22 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
11871188
case StoreOwnershipQualifier::Unqualified:
11881189
// We should not see loads with unqualified ownership when SILOwnership is
11891190
// enabled.
1190-
requireFalseOrNone(F.hasQualifiedOwnership(),
1191-
"Invalid load with unqualified ownership");
1191+
requireTrueAndSILOwnership(this, F.hasUnqualifiedOwnership(),
1192+
"Invalid load with unqualified ownership");
11921193
break;
11931194
case StoreOwnershipQualifier::Init:
11941195
case StoreOwnershipQualifier::Assign:
1195-
requireTrueOrNone(
1196-
F.hasQualifiedOwnership(),
1196+
requireTrueAndSILOwnership(
1197+
this, F.hasQualifiedOwnership(),
11971198
"Inst with qualified ownership in a function that is not qualified");
11981199
// TODO: Could probably make this a bit stricter.
11991200
require(!SI->getSrc()->getType().isTrivial(SI->getModule()),
12001201
"store [init] or store [assign] can only be applied to "
12011202
"non-trivial types");
12021203
break;
12031204
case StoreOwnershipQualifier::Trivial:
1204-
requireTrueOrNone(
1205-
F.hasQualifiedOwnership(),
1205+
requireTrueAndSILOwnership(
1206+
this, F.hasQualifiedOwnership(),
12061207
"Inst with qualified ownership in a function that is not qualified");
12071208
require(SI->getSrc()->getType().isTrivial(SI->getModule()),
12081209
"A store with trivial ownership must store a trivial type");
@@ -1352,17 +1353,19 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
13521353
void checkCopyValueInst(CopyValueInst *I) {
13531354
require(I->getOperand()->getType().isObject(),
13541355
"Source value should be an object value");
1355-
requireTrueOrNone(F.hasQualifiedOwnership(),
1356-
"copy_value is only valid in functions with qualified "
1357-
"ownership");
1356+
requireTrueAndSILOwnership(
1357+
this, F.hasQualifiedOwnership(),
1358+
"copy_value is only valid in functions with qualified "
1359+
"ownership");
13581360
}
13591361

13601362
void checkDestroyValueInst(DestroyValueInst *I) {
13611363
require(I->getOperand()->getType().isObject(),
13621364
"Source value should be an object value");
1363-
requireTrueOrNone(F.hasQualifiedOwnership(),
1364-
"destroy_value is only valid in functions with qualified "
1365-
"ownership");
1365+
requireTrueAndSILOwnership(
1366+
this, F.hasQualifiedOwnership(),
1367+
"destroy_value is only valid in functions with qualified "
1368+
"ownership");
13661369
}
13671370

13681371
void checkReleaseValueInst(ReleaseValueInst *I) {

‎lib/SILOptimizer/PassManager/Passes.cpp

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,12 +86,10 @@ bool swift::runSILDiagnosticPasses(SILModule &Module) {
8686
return Ctx.hadError();
8787
}
8888

89-
// If SILOwnership is enabled, run the ownership model eliminator.
90-
if (Module.getOptions().EnableSILOwnership) {
91-
PM.addOwnershipModelEliminator();
92-
PM.runOneIteration();
93-
PM.resetAndRemoveTransformations();
94-
}
89+
// Lower all ownership instructions right after SILGen for now.
90+
PM.addOwnershipModelEliminator();
91+
PM.runOneIteration();
92+
PM.resetAndRemoveTransformations();
9593

9694
// Otherwise run the rest of diagnostics.
9795
PM.addCapturePromotion();

‎lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,10 +151,6 @@ namespace {
151151
struct OwnershipModelEliminator : SILFunctionTransform {
152152
void run() override {
153153
SILFunction *F = getFunction();
154-
// We should only run this when SILOwnership is enabled.
155-
assert(F->getModule().getOptions().EnableSILOwnership &&
156-
"Can not run ownership model eliminator when SIL ownership is not "
157-
"enabled");
158154
bool MadeChange = false;
159155
SILBuilder B(*F);
160156
OwnershipModelEliminatorVisitor Visitor(B);

‎test/SIL/Parser/ownership_qualified_memopts.sil

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,14 @@ import Builtin
55

66
// CHECK-LABEL: sil @non_trivial : $@convention(thin) (@in Builtin.NativeObject, Builtin.NativeObject) -> () {
77
// CHECK: bb0([[ARG1:%[0-9]+]] : $*Builtin.NativeObject, [[ARG2:%[0-9]+]] : $Builtin.NativeObject):
8-
// CHECK: load [[ARG1]] : $*Builtin.NativeObject
98
// CHECK: load [take] [[ARG1]] : $*Builtin.NativeObject
109
// CHECK: load [copy] [[ARG1]] : $*Builtin.NativeObject
11-
// CHECK: store [[ARG2]] to [[ARG1]] : $*Builtin.NativeObject
1210
// CHECK: store [[ARG2]] to [init] [[ARG1]] : $*Builtin.NativeObject
1311
// CHECK: store [[ARG2]] to [assign] [[ARG1]] : $*Builtin.NativeObject
1412
sil @non_trivial : $@convention(thin) (@in Builtin.NativeObject, Builtin.NativeObject) -> () {
1513
bb0(%0 : $*Builtin.NativeObject, %1 : $Builtin.NativeObject):
16-
load %0 : $*Builtin.NativeObject
1714
load [take] %0 : $*Builtin.NativeObject
1815
load [copy] %0 : $*Builtin.NativeObject
19-
store %1 to %0 : $*Builtin.NativeObject
2016
store %1 to [init] %0 : $*Builtin.NativeObject
2117
store %1 to [assign] %0 : $*Builtin.NativeObject
2218
%2 = tuple()
@@ -25,15 +21,11 @@ bb0(%0 : $*Builtin.NativeObject, %1 : $Builtin.NativeObject):
2521

2622
// CHECK-LABEL: sil @trivial : $@convention(thin) (@in Builtin.Int32, Builtin.Int32) -> () {
2723
// CHECK: bb0([[ARG1:%[0-9]+]] : $*Builtin.Int32, [[ARG2:%[0-9]+]] : $Builtin.Int32):
28-
// CHECK: load [[ARG1]] : $*Builtin.Int32
2924
// CHECK: load [trivial] [[ARG1]] : $*Builtin.Int32
30-
// CHECK: store [[ARG2]] to [[ARG1]] : $*Builtin.Int32
3125
// CHECK: store [[ARG2]] to [trivial] [[ARG1]] : $*Builtin.Int32
3226
sil @trivial : $@convention(thin) (@in Builtin.Int32, Builtin.Int32) -> () {
3327
bb0(%0 : $*Builtin.Int32, %1 : $Builtin.Int32):
34-
load %0 : $*Builtin.Int32
3528
load [trivial] %0 : $*Builtin.Int32
36-
store %1 to %0 : $*Builtin.Int32
3729
store %1 to [trivial] %0 : $*Builtin.Int32
3830
%2 = tuple()
3931
return %2 : $()

‎test/SIL/Serialization/ownership_qualified_memopts.sil

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,34 +12,26 @@ import Builtin
1212

1313
// CHECK-LABEL: sil @trivial : $@convention(thin) (@in Builtin.Int32, Builtin.Int32) -> () {
1414
// CHECK: bb0([[ARG1:%[0-9]+]] : $*Builtin.Int32, [[ARG2:%[0-9]+]] : $Builtin.Int32):
15-
// CHECK: load [[ARG1]] : $*Builtin.Int32
1615
// CHECK: load [trivial] [[ARG1]] : $*Builtin.Int32
17-
// CHECK: store [[ARG2]] to [[ARG1]] : $*Builtin.Int32
1816
// CHECK: store [[ARG2]] to [trivial] [[ARG1]] : $*Builtin.Int32
1917
sil @trivial : $@convention(thin) (@in Builtin.Int32, Builtin.Int32) -> () {
2018
bb0(%0 : $*Builtin.Int32, %1 : $Builtin.Int32):
21-
load %0 : $*Builtin.Int32
2219
load [trivial] %0 : $*Builtin.Int32
23-
store %1 to %0 : $*Builtin.Int32
2420
store %1 to [trivial] %0 : $*Builtin.Int32
2521
%2 = tuple()
2622
return %2 : $()
2723
}
2824

2925
// CHECK-LABEL: sil @non_trivial : $@convention(thin) (@in Builtin.NativeObject, Builtin.NativeObject) -> () {
3026
// CHECK: bb0([[ARG1:%[0-9]+]] : $*Builtin.NativeObject, [[ARG2:%[0-9]+]] : $Builtin.NativeObject):
31-
// CHECK: load [[ARG1]] : $*Builtin.NativeObject
3227
// CHECK: load [take] [[ARG1]] : $*Builtin.NativeObject
3328
// CHECK: load [copy] [[ARG1]] : $*Builtin.NativeObject
34-
// CHECK: store [[ARG2]] to [[ARG1]] : $*Builtin.NativeObject
3529
// CHECK: store [[ARG2]] to [init] [[ARG1]] : $*Builtin.NativeObject
3630
// CHECK: store [[ARG2]] to [assign] [[ARG1]] : $*Builtin.NativeObject
3731
sil @non_trivial : $@convention(thin) (@in Builtin.NativeObject, Builtin.NativeObject) -> () {
3832
bb0(%0 : $*Builtin.NativeObject, %1 : $Builtin.NativeObject):
39-
load %0 : $*Builtin.NativeObject
4033
load [take] %0 : $*Builtin.NativeObject
4134
load [copy] %0 : $*Builtin.NativeObject
42-
store %1 to %0 : $*Builtin.NativeObject
4335
store %1 to [init] %0 : $*Builtin.NativeObject
4436
store %1 to [assign] %0 : $*Builtin.NativeObject
4537
%2 = tuple()

0 commit comments

Comments
 (0)