Skip to content

Commit 1d13039

Browse files
author
jturcotti
committed
add more comprehensive cases to tests, and fix many bugs, relying on much more potent resolution of addresses
1 parent 1a716d8 commit 1d13039

File tree

3 files changed

+256
-41
lines changed

3 files changed

+256
-41
lines changed

include/swift/SILOptimizer/Utils/PartitionUtils.h

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -86,21 +86,37 @@ class PartitionOp {
8686
}
8787

8888
void dump() const LLVM_ATTRIBUTE_USED {
89+
raw_ostream &os = llvm::errs();
8990
switch (OpKind) {
9091
case PartitionOpKind::Assign:
91-
llvm::dbgs() << "assign %%" << OpArgs[0] << " = %%" << OpArgs[1] << "\n";
92+
os.changeColor(llvm::raw_ostream::CYAN, true);
93+
os << "assign";
94+
os.resetColor();
95+
os << " %%" << OpArgs[0] << " = %%" << OpArgs[1] << "\n";
9296
break;
9397
case PartitionOpKind::AssignFresh:
94-
llvm::dbgs() << "assign_fresh %%" << OpArgs[0] << "\n";
98+
os.changeColor(llvm::raw_ostream::GREEN, true);
99+
os << "assign_fresh";
100+
os.resetColor();
101+
os << " %%" << OpArgs[0] << "\n";
95102
break;
96103
case PartitionOpKind::Consume:
97-
llvm::dbgs() << "consume %%" << OpArgs[0] << "\n";
104+
os.changeColor(llvm::raw_ostream::RED, true);
105+
os << "consume";
106+
os.resetColor();
107+
os << " %%" << OpArgs[0] << "\n";
98108
break;
99109
case PartitionOpKind::Merge:
100-
llvm::dbgs() << "merge %%" << OpArgs[0] << " with %%" << OpArgs[1] << "\n";
110+
os.changeColor(llvm::raw_ostream::BLUE, true);
111+
os << "merge";
112+
os.resetColor();
113+
os << " %%" << OpArgs[0] << " with %%" << OpArgs[1] << "\n";
101114
break;
102115
case PartitionOpKind::Require:
103-
llvm::dbgs() << "require %%" << OpArgs[0] << "\n";
116+
os.changeColor(llvm::raw_ostream::YELLOW, true);
117+
os << "require";
118+
os.resetColor();
119+
os << " %%" << OpArgs[0] << "\n";
104120
break;
105121
}
106122
}
@@ -160,10 +176,10 @@ class Partition {
160176
if (label < 0) continue;
161177

162178
// this label should not exceed fresh_label
163-
if (label >= fresh_label) return fail(i, 0);
179+
if ((unsigned) label >= fresh_label) return fail(i, 0);
164180

165181
// the label of a region should be at most as large as each index in it
166-
if (i < label) return fail(i, 1);
182+
if ((unsigned) label > i) return fail(i, 1);
167183

168184
// each region label should refer to an index in that region
169185
if (labels[label] != label) return fail(i, 2);
@@ -299,10 +315,21 @@ class Partition {
299315
PartitionOp op,
300316
llvm::function_ref<void(const PartitionOp&, unsigned)>
301317
handleFailure = [](const PartitionOp&, unsigned) {},
302-
std::vector<unsigned> nonconsumables = {},
318+
319+
std::vector<unsigned>
320+
nonconsumables = {},
321+
303322
llvm::function_ref<void(const PartitionOp&, unsigned)>
304-
handleConsumeNonConsumable = [](const PartitionOp&, unsigned) {}
323+
handleConsumeNonConsumable = [](const PartitionOp&, unsigned) {},
324+
325+
bool reviveAfterFailure = false
305326
) {
327+
auto handleFailureAndRevive =
328+
[&](const PartitionOp& partitionOp, unsigned consumedVal) {
329+
if (reviveAfterFailure)
330+
horizontalUpdate(labels, consumedVal, fresh_label++);
331+
handleFailure(partitionOp, consumedVal);
332+
};
306333
switch (op.OpKind) {
307334
case PartitionOpKind::Assign:
308335
assert(op.OpArgs.size() == 2 &&
@@ -311,7 +338,7 @@ class Partition {
311338
"Assign PartitionOp's source argument should be already tracked");
312339
// if assigning to a missing region, handle the failure
313340
if (labels[op.OpArgs[1]] < 0)
314-
handleFailure(op, op.OpArgs[1]);
341+
handleFailureAndRevive(op, op.OpArgs[1]);
315342

316343
labels[op.OpArgs[0]] = labels[op.OpArgs[1]];
317344

@@ -322,8 +349,6 @@ class Partition {
322349
case PartitionOpKind::AssignFresh:
323350
assert(op.OpArgs.size() == 1 &&
324351
"AssignFresh PartitionOp should be passed 1 argument");
325-
assert(!labels.count(op.OpArgs[0]) &&
326-
"AssignFresh PartitionOp's argument should NOT already be tracked");
327352

328353
// map index op.OpArgs[0] to a fresh label
329354
labels[op.OpArgs[0]] = fresh_label++;
@@ -337,7 +362,7 @@ class Partition {
337362

338363
// if attempting to consume a consumed region, handle the failure
339364
if (labels[op.OpArgs[0]] < 0)
340-
handleFailure(op, op.OpArgs[0]);
365+
handleFailureAndRevive(op, op.OpArgs[0]);
341366

342367
// mark region as consumed
343368
horizontalUpdate(labels, op.OpArgs[0], -1);
@@ -360,11 +385,12 @@ class Partition {
360385
"Merge PartitionOp should be passed 2 arguments");
361386
assert(labels.count(op.OpArgs[0]) && labels.count(op.OpArgs[1]) &&
362387
"Merge PartitionOp's arguments should already be tracked");
388+
363389
// if attempting to merge a consumed region, handle the failure
364390
if (labels[op.OpArgs[0]] < 0)
365-
handleFailure(op, op.OpArgs[0]);
391+
handleFailureAndRevive(op, op.OpArgs[0]);
366392
if (labels[op.OpArgs[1]] < 0)
367-
handleFailure(op, op.OpArgs[1]);
393+
handleFailureAndRevive(op, op.OpArgs[1]);
368394

369395
merge(op.OpArgs[0], op.OpArgs[1]);
370396
break;
@@ -374,7 +400,7 @@ class Partition {
374400
assert(labels.count(op.OpArgs[0]) &&
375401
"Require PartitionOp's argument should already be tracked");
376402
if (labels[op.OpArgs[0]] < 0)
377-
handleFailure(op, op.OpArgs[0]);
403+
handleFailureAndRevive(op, op.OpArgs[0]);
378404
}
379405

380406
assert(is_canonical_correct());

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

Lines changed: 93 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include "swift/SIL/BasicBlockData.h"
77
#include "swift/SIL/BasicBlockDatastructures.h"
88
#include "swift/SIL/MemAccessUtils.h"
9+
#include "swift/SIL/OwnershipUtils.h"
910
#include "swift/SIL/SILBasicBlock.h"
1011
#include "swift/SIL/SILFunction.h"
1112
#include "swift/SIL/SILInstruction.h"
@@ -58,33 +59,96 @@ class PartitionOpTranslator {
5859
ProtocolDecl *sendableProtocol;
5960

6061
// nodeIDMap stores unique IDs for all SILNodes corresponding to
61-
// non-Sendable values. Implicit conversion from SILValue used pervasively
62+
// non-Sendable values. Implicit conversion from SILValue used pervasively.
63+
// ensure simplifyVal is called on SILValues before entering into this map
6264
llvm::DenseMap<const SILNode *, unsigned> nodeIDMap;
6365
unsigned nextNodeID = 0;
6466

67+
// some values that AccessStorage claims are uniquely identified are still
68+
// captured (e.g. in a closure). This set is initialized upon
69+
// PartitionOpTranslator construction to store those values.
70+
// ensure simplifyVal is called on SILValues before entering into this map
71+
//
72+
// TODO: we could remember not just which values fit this description,
73+
// but at what points in function flow they do, this would be more
74+
// permissive, but I'm avoiding implementing it in case existing
75+
// utilities would make it easier than handrolling
76+
std::set<SILValue> capturedUIValues;
77+
78+
void initCapturedUIValues() {
79+
for (const SILBasicBlock &block: *function) {
80+
for (const SILInstruction &inst: block) {
81+
if (isApplyInst(inst)) {
82+
// add all nonsendable, uniquely identified arguments to applications
83+
// to capturedUIValues, because applications capture them
84+
for (SILValue val : inst.getOperandValues()) {
85+
if (isNonSendable(val) && isUniquelyIdentified(val))
86+
capturedUIValues.insert(simplifyVal(val));
87+
}
88+
}
89+
}
90+
}
91+
}
92+
93+
public:
94+
// create a new PartitionOpTranslator, all that's needed is the underlying
95+
// SIL function
96+
PartitionOpTranslator(SILFunction *function) :
97+
function(function),
98+
sendableProtocol(function->getASTContext()
99+
.getProtocol(KnownProtocolKind::Sendable)) {
100+
assert(sendableProtocol && "PartitionOpTranslators should only be created "
101+
"in contexts in which the availability of the "
102+
"Sendable protocol has already been checked.");
103+
initCapturedUIValues();
104+
LLVM_DEBUG(
105+
llvm::dbgs() << "Captured Uniquely Identified addresses for "
106+
<< function->getName() << ":\n";
107+
for (SILValue val : capturedUIValues)
108+
val->dump();
109+
110+
);
111+
}
112+
113+
private:
114+
static inline bool isAddress(SILValue val) {
115+
return val->getType().isAddress();
116+
}
117+
118+
static bool isApplyInst(const SILInstruction &inst) {
119+
return isa<ApplyInst, TryApplyInst, PartialApplyInst, BuiltinInst>(inst);
120+
}
121+
65122
AccessStorage getAccessStorageFromAddr(SILValue val) {
66-
assert(val->getType().isAddress());
123+
assert(isAddress(val));
67124
auto accessStorage = AccessStorage::compute(val);
68125
if (accessStorage) {
69-
if (auto initExistential = dyn_cast_or_null<InitExistentialAddrInst>(
70-
accessStorage.getRoot().getDefiningInstruction()))
126+
auto definingInst = accessStorage.getRoot().getDefiningInstruction();
127+
if (definingInst &&
128+
isa<InitExistentialAddrInst, CopyValueInst>(definingInst))
71129
// look through these because AccessStorage does not
72-
return getAccessStorageFromAddr(initExistential->getOperand());
130+
return getAccessStorageFromAddr(definingInst->getOperand(0));
73131
}
74132
return accessStorage;
75133
}
76134

77135
bool isUniquelyIdentified(SILValue val) {
78-
if (!val->getType().isAddress())
136+
val = simplifyVal(val);
137+
if (!isAddress(val))
79138
return false;
80-
if (auto accessStorage = getAccessStorageFromAddr(val)) {
81-
return accessStorage.isUniquelyIdentified();
82-
}
139+
if (auto accessStorage = getAccessStorageFromAddr(val))
140+
return accessStorage.isUniquelyIdentified() &&
141+
!capturedUIValues.count(simplifyVal(val));
83142
return false;
84143
}
85144

145+
// simplifyVal reduces an address-typed SILValue to the root SILValue
146+
// that it was derived from, reducing the set of values that must be
147+
// reasoned about by rendering two values that are projections/aliases the
148+
// same.
149+
// TODO: make usage of this more principled with a wrapper type SimplSILValue
86150
SILValue simplifyVal(SILValue val) {
87-
if (!val->getType().isAddress())
151+
if (!isAddress(val))
88152
return getUnderlyingObject(val);
89153
if (auto accessStorage = getAccessStorageFromAddr(val)) {
90154
return accessStorage.getRoot();
@@ -209,17 +273,6 @@ class PartitionOpTranslator {
209273
}
210274

211275
public:
212-
// create a new PartitionOpTranslator, all that's needed is the underlying
213-
// SIL function
214-
PartitionOpTranslator(SILFunction *function) :
215-
function(function),
216-
sendableProtocol(function->getASTContext()
217-
.getProtocol(KnownProtocolKind::Sendable)) {
218-
assert(sendableProtocol && "PartitionOpTranslators should only be created "
219-
"in contexts in which the availability of the "
220-
"Sendable protocol has already been checked.");
221-
}
222-
223276
// Create a partition that places all arguments from this function,
224277
// including self if available, into the same region, ensuring those
225278
// arguments get IDs in doing so. This Partition will be used as the
@@ -352,17 +405,22 @@ class PartitionOpTranslator {
352405
}
353406
// ===========================================================================
354407

408+
// used to index the translations of SILInstructions performed
409+
int translationIndex = 0;
410+
355411
// Some SILInstructions contribute to the partition of non-Sendable values
356412
// being analyzed. translateSILInstruction translate a SILInstruction
357413
// to its effect on the non-Sendable partition, if it has one.
358414
//
359415
// The current pattern of
360416
std::vector<PartitionOp> translateSILInstruction(SILInstruction *instruction) {
417+
translationIndex++;
361418
currentInstruction = instruction;
362419

363420
// The following instructions are treated as assigning their result to a
364421
// fresh region.
365-
if (isa<AllocRefInst,
422+
if (isa<AllocBoxInst,
423+
AllocRefInst,
366424
AllocStackInst,
367425
LiteralInst>(instruction)) {
368426
return translateSILAssignFresh(instruction->getResult(0));
@@ -410,7 +468,7 @@ class PartitionOpTranslator {
410468
}
411469

412470
// Handle applications
413-
if (isa<ApplyInst, PartialApplyInst>(instruction)) {
471+
if (isApplyInst(*instruction)) {
414472
return translateSILApply(instruction);
415473
}
416474

@@ -428,6 +486,11 @@ class PartitionOpTranslator {
428486
return translateSILRequire(returnInst->getOperand());
429487
}
430488

489+
LLVM_DEBUG(
490+
llvm::dbgs() << "WARN: unhandled instruction kind "
491+
<< getSILInstructionName(instruction->getKind());
492+
);
493+
431494
return {};
432495
}
433496

@@ -454,8 +517,12 @@ class PartitionOpTranslator {
454517
partitionOps.push_back(op);
455518

456519
LLVM_DEBUG(
520+
llvm::dbgs() << " ┌─┬─╼";
457521
instruction.dump();
458-
llvm::dbgs() << " └───╼ ";
522+
llvm::dbgs() << " │ └─╼ ";
523+
instruction.getLoc().getSourceLoc().printLineAndColumn(llvm::dbgs(), function->getASTContext().SourceMgr);
524+
llvm::dbgs() << " │ translation #" << translationIndex;
525+
llvm::dbgs() << "\n └─────╼ ";
459526
op.dump();
460527
);
461528
}
@@ -527,7 +594,8 @@ class BlockPartitionState {
527594
for (auto &partitionOp : blockPartitionOps) {
528595
workingPartition.apply(partitionOp, handleFailure,
529596
translator.getNonConsumables(),
530-
handleConsumeNonConsumable);
597+
handleConsumeNonConsumable,
598+
/*reviveAfterFailure=*/ false);
531599
}
532600
}
533601

0 commit comments

Comments
 (0)