Skip to content

Commit 2336a87

Browse files
atrickatrick
committed
[Exclusivity] Enable access markers for the entire -Onone pipeline.
Dynamic markers are still conditional on the command line option.
1 parent 8187aae commit 2336a87

File tree

9 files changed

+210
-109
lines changed

9 files changed

+210
-109
lines changed

‎include/swift/AST/DiagnosticsFrontend.def

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ ERROR(error_unsupported_opt_for_target, none,
5858
ERROR(error_argument_not_allowed_with, none,
5959
"argument '%0' is not allowed with '%1'", (StringRef, StringRef))
6060

61+
WARNING(warning_argument_not_supported_with_optimization, none,
62+
"argument '%0' is not supported with optimization", (StringRef))
63+
6164
ERROR(error_option_requires_sanitizer, none,
6265
"option '%0' requires a sanitizer to be enabled. Use -sanitize= to enable a"
6366
" sanitizer", (StringRef))

‎lib/Frontend/CompilerInvocation.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1237,6 +1237,12 @@ void parseExclusivityEnforcementOptions(const llvm::opt::Arg *A,
12371237
Diags.diagnose(SourceLoc(), diag::error_unsupported_option_argument,
12381238
A->getOption().getPrefixedName(), A->getValue());
12391239
}
1240+
if (Opts.Optimization > SILOptions::SILOptMode::None
1241+
&& Opts.EnforceExclusivityDynamic) {
1242+
Diags.diagnose(SourceLoc(),
1243+
diag::warning_argument_not_supported_with_optimization,
1244+
A->getOption().getPrefixedName() + A->getValue());
1245+
}
12401246
}
12411247

12421248
static bool ParseSILArgs(SILOptions &Opts, ArgList &Args,

‎lib/SILOptimizer/Mandatory/AccessEnforcementSelection.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@
1212
///
1313
/// This pass eliminates 'unknown' access enforcement by selecting either
1414
/// static or dynamic enforcement.
15-
///
15+
///
16+
/// FIXME: handle boxes used by copy_value when neither copy is captured.
17+
///
1618
//===----------------------------------------------------------------------===//
1719

1820
#define DEBUG_TYPE "access-enforcement-selection"
@@ -110,6 +112,8 @@ void SelectEnforcement::run() {
110112
updateAccesses();
111113
}
112114

115+
// FIXME: This should cover a superset of AllocBoxToStack's findUnexpectedBoxUse
116+
// to avoid perturbing codegen. They should be sharing the same analysis.
113117
void SelectEnforcement::analyzeUsesOfBox(SILInstruction *source) {
114118
// Collect accesses rooted off of projections.
115119
for (auto use : source->getUses()) {
@@ -137,7 +141,7 @@ void SelectEnforcement::analyzeUsesOfBox(SILInstruction *source) {
137141
// Add it to the escapes set.
138142
Escapes.insert(user);
139143

140-
//
144+
// Record this point as escaping.
141145
auto userBB = user->getParent();
142146
auto &state = StateMap[userBB];
143147
if (!state.IsInWorklist) {

‎lib/SILOptimizer/Mandatory/AccessMarkerElimination.cpp

Lines changed: 131 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -28,108 +28,155 @@
2828

2929
using namespace swift;
3030

31+
// This option allows markers to remain in -Onone as a structural SIL property.
32+
// Regardless of this option, sufficient markers are always emitted to satisfy
33+
// the current enforcement level. This options simply allows markers to remain
34+
// for testing and validation.
35+
//
36+
// This option only applies to InactiveAccessMarkerElimination. Any occurrence
37+
// of FullAccessMarkerElimination in the pass pipeline effectively overrides the
38+
// options and removes all markers.
39+
//
40+
// At -Onone, with EnableMarkers, no static markers are removed.
41+
// With !EnableMarkers:
42+
// Enforcement | Static | Dynamic
43+
// none | Remove after Diag | Remove ASAP
44+
// unchecked | Remain through IRGen | Remove ASAP
45+
// checked | Remain through IRGen | Remain through IRGen
46+
// dynamic-only| Remove after Diag | Remain through IRGen
47+
llvm::cl::opt<bool> EnableAccessMarkers(
48+
"sil-access-markers", llvm::cl::init(true),
49+
llvm::cl::desc("Enable memory access makers that aren't needed for "
50+
"diagnostics."));
51+
3152
namespace {
3253

3354
struct AccessMarkerElimination : SILModuleTransform {
3455
virtual bool isFullElimination() = 0;
3556

36-
void replaceBeginAccessUsers(BeginAccessInst *beginAccess) {
57+
bool removedAny = false;
58+
59+
SILBasicBlock::iterator eraseInst(SILInstruction *inst) {
60+
removedAny = true;
61+
return inst->getParent()->erase(inst);
62+
};
3763

38-
// Handle all the uses:
39-
while (!beginAccess->use_empty()) {
40-
Operand *op = *beginAccess->use_begin();
64+
void replaceBeginAccessUsers(BeginAccessInst *beginAccess);
4165

42-
// Delete any associated end_access instructions.
43-
if (auto endAccess = dyn_cast<EndAccessInst>(op->getUser())) {
44-
assert(endAccess->use_empty() && "found use of end_access");
45-
endAccess->eraseFromParent();
66+
// Precondition: !EnableAccessMarkers || isFullElimination()
67+
bool shouldPreserveAccess(SILAccessEnforcement enforcement);
4668

47-
// Forward all other uses to the original address.
48-
} else {
49-
op->set(beginAccess->getSource());
50-
}
69+
// Check if the instruction is a marker that should be eliminated. If so,
70+
// updated the SIL, short of erasing the marker itself, and return true.
71+
bool checkAndEliminateMarker(SILInstruction *inst);
72+
73+
// Entry point called by the pass manager.
74+
void run() override;
75+
};
76+
77+
void AccessMarkerElimination::replaceBeginAccessUsers(
78+
BeginAccessInst *beginAccess) {
79+
// Handle all the uses:
80+
while (!beginAccess->use_empty()) {
81+
Operand *op = *beginAccess->use_begin();
82+
83+
// Delete any associated end_access instructions.
84+
if (auto endAccess = dyn_cast<EndAccessInst>(op->getUser())) {
85+
assert(endAccess->use_empty() && "found use of end_access");
86+
endAccess->eraseFromParent();
87+
88+
// Forward all other uses to the original address.
89+
} else {
90+
op->set(beginAccess->getSource());
5191
}
5292
}
93+
}
94+
95+
// Precondition: !EnableAccessMarkers || isFullElimination()
96+
bool AccessMarkerElimination::shouldPreserveAccess(
97+
SILAccessEnforcement enforcement) {
98+
if (isFullElimination())
99+
return false;
100+
101+
auto &M = *getModule();
102+
switch (enforcement) {
103+
case SILAccessEnforcement::Unknown:
104+
return false;
105+
case SILAccessEnforcement::Static:
106+
// Even though static enforcement is already performed, this flag is
107+
// useful to control marker preservation for now.
108+
return EnableAccessMarkers || M.getOptions().EnforceExclusivityStatic;
109+
case SILAccessEnforcement::Dynamic:
110+
// FIXME: when dynamic markers are fully supported, don't strip:
111+
// return EnableAccessMarkers || M.getOptions().EnforceExclusivityDynamic;
112+
return M.getOptions().EnforceExclusivityDynamic;
113+
case SILAccessEnforcement::Unsafe:
114+
return false;
115+
}
116+
}
53117

54-
bool shouldPreserveAccess(SILAccessEnforcement enforcement) {
55-
auto &M = *getModule();
56-
switch (enforcement) {
57-
case SILAccessEnforcement::Unknown:
118+
// Check if the instruction is a marker that should be eliminated. If so,
119+
// updated the SIL, short of erasing the marker itself, and return true.
120+
bool AccessMarkerElimination::checkAndEliminateMarker(SILInstruction *inst) {
121+
if (auto beginAccess = dyn_cast<BeginAccessInst>(inst)) {
122+
// Leave dynamic accesses in place, but delete all others.
123+
if (shouldPreserveAccess(beginAccess->getEnforcement()))
58124
return false;
59-
case SILAccessEnforcement::Static:
60-
// Even though static enforcement is already performed, this flag is
61-
// useful to control marker preservation for now.
62-
return M.getOptions().EnforceExclusivityStatic;
63-
case SILAccessEnforcement::Dynamic:
64-
return M.getOptions().EnforceExclusivityDynamic;
65-
case SILAccessEnforcement::Unsafe:
125+
126+
replaceBeginAccessUsers(beginAccess);
127+
return true;
128+
}
129+
130+
// end_access instructions will be handled when we process the
131+
// begin_access.
132+
133+
// begin_unpaired_access instructions will be directly removed and
134+
// simply replaced with their operand.
135+
if (auto BUA = dyn_cast<BeginUnpairedAccessInst>(inst)) {
136+
if (shouldPreserveAccess(BUA->getEnforcement()))
66137
return false;
67-
}
68-
};
69138

70-
void run() override {
71-
auto &M = *getModule();
72-
73-
bool removedAny = false;
74-
75-
auto eraseInst = [&removedAny](SILInstruction *inst) {
76-
removedAny = true;
77-
return inst->getParent()->erase(inst);
78-
};
79-
80-
for (auto &F : M) {
81-
// Iterating in reverse eliminates more begin_access users before they
82-
// need to be replaced.
83-
for (auto &BB : reversed(F)) {
84-
// Don't cache the begin iterator since we're reverse iterating.
85-
for (auto II = BB.end(); II != BB.begin();) {
86-
SILInstruction *inst = &*(--II);
87-
88-
if (auto beginAccess = dyn_cast<BeginAccessInst>(inst)) {
89-
// Leave dynamic accesses in place, but delete all others.
90-
if (shouldPreserveAccess(beginAccess->getEnforcement()))
91-
continue;
92-
93-
replaceBeginAccessUsers(beginAccess);
94-
II = eraseInst(beginAccess);
95-
continue;
96-
}
97-
98-
// end_access instructions will be handled when we process the
99-
// begin_access.
100-
101-
// begin_unpaired_access instructions will be directly removed and
102-
// simply replaced with their operand.
103-
if (auto BUA = dyn_cast<BeginUnpairedAccessInst>(inst)) {
104-
if (shouldPreserveAccess(BUA->getEnforcement()))
105-
continue;
106-
107-
BUA->replaceAllUsesWith(BUA->getSource());
108-
II = eraseInst(BUA);
109-
continue;
110-
}
111-
// end_unpaired_access instructions will be directly removed and
112-
// simply replaced with their operand.
113-
if (auto EUA = dyn_cast<EndUnpairedAccessInst>(inst)) {
114-
if (shouldPreserveAccess(EUA->getEnforcement()))
115-
continue;
116-
117-
assert(EUA->use_empty() && "use of end_unpaired_access");
118-
II = eraseInst(EUA);
119-
continue;
120-
}
121-
}
122-
}
139+
BUA->replaceAllUsesWith(BUA->getSource());
140+
return true;
141+
}
142+
// end_unpaired_access instructions will be directly removed and
143+
// simply replaced with their operand.
144+
if (auto EUA = dyn_cast<EndUnpairedAccessInst>(inst)) {
145+
if (shouldPreserveAccess(EUA->getEnforcement()))
146+
return false;
123147

124-
// Don't invalidate any analyses if we didn't do anything.
125-
if (!removedAny)
126-
continue;
148+
assert(EUA->use_empty() && "use of end_unpaired_access");
149+
return true;
150+
}
151+
return false;
152+
}
127153

128-
auto InvalidKind = SILAnalysis::InvalidationKind::Instructions;
129-
invalidateAnalysis(&F, InvalidKind);
154+
void AccessMarkerElimination::run() {
155+
// FIXME: When dynamic markers are fully supported, just skip this pass:
156+
// if (EnableAccessMarkers && !isFullElimination())
157+
// return;
158+
159+
auto &M = *getModule();
160+
for (auto &F : M) {
161+
// Iterating in reverse eliminates more begin_access users before they
162+
// need to be replaced.
163+
for (auto &BB : reversed(F)) {
164+
// Don't cache the begin iterator since we're reverse iterating.
165+
for (auto II = BB.end(); II != BB.begin();) {
166+
SILInstruction *inst = &*(--II);
167+
if (checkAndEliminateMarker(inst))
168+
II = eraseInst(inst);
169+
}
130170
}
171+
172+
// Don't invalidate any analyses if we didn't do anything.
173+
if (!removedAny)
174+
continue;
175+
176+
auto InvalidKind = SILAnalysis::InvalidationKind::Instructions;
177+
invalidateAnalysis(&F, InvalidKind);
131178
}
132-
};
179+
}
133180

134181
struct InactiveAccessMarkerElimination : AccessMarkerElimination {
135182
virtual bool isFullElimination() { return false; }

‎lib/SILOptimizer/PassManager/PassPipeline.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,13 +79,17 @@ static void addMandatoryOptPipeline(SILPassPipelinePlan &P,
7979
}
8080
P.addDiagnoseStaticExclusivity();
8181
P.addCapturePromotion();
82+
83+
// Select access kind after capture promotion and before stack promotion.
84+
// This guarantees that stack-promotable boxes have [static] enforcement.
85+
P.addAccessEnforcementSelection();
86+
P.addInactiveAccessMarkerElimination();
87+
8288
P.addAllocBoxToStack();
8389
P.addNoReturnFolding();
8490
P.addOwnershipModelEliminator();
8591
P.addMarkUninitializedFixup();
8692
P.addDefiniteInitialization();
87-
P.addAccessEnforcementSelection();
88-
P.addInactiveAccessMarkerElimination();
8993
P.addMandatoryInlining();
9094
P.addPredictableMemoryOptimizations();
9195
P.addDiagnosticConstantPropagation();

‎lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,7 @@ static void replaceProjectBoxUsers(SILValue HeapBox, SILValue StackBox) {
435435
while (!Worklist.empty()) {
436436
auto *Op = Worklist.pop_back_val();
437437
if (auto *PBI = dyn_cast<ProjectBoxInst>(Op->getUser())) {
438+
// This may result in an alloc_stack being used by begin_access [dymaic].
438439
PBI->replaceAllUsesWith(StackBox);
439440
continue;
440441
}

‎test/SILOptimizer/access_enforcement_selection.swift

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ public func takesInout(_ i: inout Int) {
77
i = 42
88
}
99
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection10takesInoutySizF
10-
// CHECK: Static Access: %{{.*}} = begin_access [modify] [static] %0 : $*Int
10+
// CHECK: Static Access: %{{.*}} = begin_access [modify] [static] %{{.*}} : $*Int
1111

1212
// Helper taking a basic, no-escape closure.
1313
func takeClosure(_: ()->Int) {}
@@ -22,14 +22,14 @@ public func captureStack() -> Int {
2222
}
2323
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection12captureStackSiyF
2424
// Static access for `return x`.
25-
// CHECK: Static Access: %{{.*}} = begin_access [read] [static] %0 : $*Int
25+
// CHECK: Static Access: %{{.*}} = begin_access [read] [static] %{{.*}} : $*Int
2626

2727
// The access inside the closure is dynamic, until we have the logic necessary to
2828
// prove that no other closures are passed to `takeClosure` that may write to
2929
// `x`.
3030
//
3131
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection12captureStackSiyFSiycfU_
32-
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %0 : $*Int
32+
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int
3333

3434
// Generate a closure in which the @inout_aliasing argument
3535
// escapes to an @inout function `bar`.
@@ -41,27 +41,15 @@ public func recaptureStack() -> Int {
4141
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14recaptureStackSiyF
4242
//
4343
// Static access for `return x`.
44-
// CHECK: Static Access: %10 = begin_access [read] [static] %0 : $*Int
44+
// CHECK: Static Access: %{{.*}} = begin_access [read] [static] %{{.*}} : $*Int
4545

4646
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection14recaptureStackSiyFSiycfU_
4747
//
4848
// The first [modify] access inside the closure must be dynamic. It enforces the
4949
// @inout argument.
50-
// CHECK: Dynamic Access: %{{.*}} = begin_access [modify] [dynamic] %0 : $*Int
50+
// CHECK: Dynamic Access: %{{.*}} = begin_access [modify] [dynamic] %{{.*}} : $*Int
5151
//
5252
// The second [read] access is only dynamic because the analysis isn't strong
5353
// enough to prove otherwise. Same as `captureStack` above.
5454
//
55-
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %0 : $*Int
56-
57-
public func undefStack(i: Int) -> Int {
58-
_preconditionFailure("unreachable")
59-
var x = 3
60-
if i != 0 {
61-
x = 42
62-
}
63-
return x
64-
}
65-
// CHECK-LABEL: Access Enforcement Selection in _T028access_enforcement_selection10undefStackS2i1i_tF
66-
// CHECK: Static Access: %{{.*}} = begin_access [modify] [static] undef : $*Int
67-
// CHECK: Static Access: %{{.*}} = begin_access [read] [static] undef : $*Int
55+
// CHECK: Dynamic Access: %{{.*}} = begin_access [read] [dynamic] %{{.*}} : $*Int

0 commit comments

Comments
 (0)