Skip to content

[region-based-isolation] Stack of smaller fixes before a larger fix #69342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Oct 24, 2023
Merged

[region-based-isolation] Stack of smaller fixes before a larger fix #69342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d598da5
[sil] Implement SILType::isActor().
gottesmm Oct 2, 2023
c474f6c
[region-isolation] Move unittests from SIL to SILOptimizer.
gottesmm Oct 3, 2023
fb7b4a3
[region-isolation] Add verbose logging to PartitionUtils behind a flag.
gottesmm Oct 6, 2023
578eafd
[region-isolation] Eliminate a temporary std::vector.
gottesmm Oct 6, 2023
a8886e7
Delete an incorrect assert
gottesmm Oct 10, 2023
cbc1241
[region-isolation] Eliminate unnecessary helper.
gottesmm Oct 10, 2023
01b1475
[region-isolation] Emit errors directly in ConsumeRequireAccumulator …
gottesmm Oct 3, 2023
2c0efe6
[region-isolation] Go through the implementation again changing Consu…
gottesmm Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/swift/AST/DiagnosticsSIL.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,12 +877,12 @@ NOTE(sil_referencebinding_inout_binding_here, none,

// Warnings arising from the flow-sensitive checking of Sendability of
// non-Sendable values
WARNING(arg_region_consumed, none,
WARNING(arg_region_transferred, none,
"call site passes `self` or a non-sendable argument of this function to another thread, potentially yielding a race with the caller", ())
WARNING(consumption_yields_race, none,
WARNING(transfer_yields_race, none,
"non-sendable value sent across isolation domains that could be concurrently accessed later in this function (%0 access site%select{|s}1 displayed%select{|, %3 more hidden}2)",
(unsigned, bool, bool, unsigned))
WARNING(call_site_consumption_yields_race, none,
WARNING(call_site_transfer_yields_race, none,
"passing argument of non-sendable type %0 from %1 context to %2 context at this call site could yield a race with accesses later in this function (%3 access site%select{|s}4 displayed%select{|, %6 more hidden}5)",
(Type, ActorIsolation, ActorIsolation, unsigned, bool, bool, unsigned))
NOTE(possible_racy_access_site, none,
Expand Down
2 changes: 2 additions & 0 deletions include/swift/SIL/SILType.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,6 +878,8 @@ class SILType {

bool isMarkedAsImmortal() const;

bool isActor() const { return getASTType()->isActorType(); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not name it isActorType() for consistency with the CanType method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that we already know it is a type, so no point in adding that redundant text.


ProtocolConformanceRef conformsToProtocol(SILFunction *fn,
ProtocolDecl *protocol) const;

Expand Down
124 changes: 72 additions & 52 deletions include/swift/SILOptimizer/Utils/PartitionUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef SWIFT_PARTITIONUTILS_H
#define SWIFT_PARTITIONUTILS_H

#include "swift/Basic/Defer.h"
#include "swift/Basic/LLVM.h"
#include "swift/SIL/SILInstruction.h"
#include "llvm/ADT/SmallVector.h"
Expand All @@ -13,6 +14,18 @@ namespace swift {

namespace PartitionPrimitives {

#ifndef NDEBUG
extern bool REGIONBASEDISOLATION_ENABLE_VERBOSE_LOGGING;
#define REGIONBASEDISOLATION_VERBOSE_LOG(...) \
do { \
if (REGIONBASEDISOLATION_ENABLE_VERBOSE_LOGGING) { \
LLVM_DEBUG(__VA_ARGS__); \
} \
} while (0);
#else
#define REGIONBASEDISOLATION_VERBOSE_LOG(...)
#endif

struct Element {
unsigned num;

Expand All @@ -36,9 +49,9 @@ struct Region {

operator signed() const { return num; }

bool isConsumed() const { return num < 0; }
bool isTransferred() const { return num < 0; }

static Region consumed() { return Region(-1); }
static Region transferred() { return Region(-1); }
};
}

Expand All @@ -48,20 +61,20 @@ using namespace PartitionPrimitives;
/// SILInstructions can be translated to
enum class PartitionOpKind : uint8_t {
/// Assign one value to the region of another, takes two args, second arg
/// must already be tracked with a non-consumed region
/// must already be tracked with a non-transferred region
Assign,

/// Assign one value to a fresh region, takes one arg.
AssignFresh,

/// Merge the regions of two values, takes two args, both must be from
/// non-consumed regions.
/// non-transferred regions.
Merge,

/// Consume the region of a value if not already consumed, takes one arg.
/// Consume the region of a value if not already transferred, takes one arg.
Transfer,

/// Require the region of a value to be non-consumed, takes one arg.
/// Require the region of a value to be non-transferred, takes one arg.
Require,
};

Expand All @@ -79,7 +92,8 @@ class PartitionOp {
SILInstruction *sourceInst;

// Record an AST expression corresponding to this PartitionOp, currently
// populated only for Consume expressions to indicate the value being consumed
// populated only for Consume expressions to indicate the value being
// transferred
Expr *sourceExpr;

// TODO: can the following declarations be merged?
Expand Down Expand Up @@ -163,7 +177,7 @@ class PartitionOp {
os << "assign_fresh %%" << OpArgs[0] << "\n";
break;
case PartitionOpKind::Transfer:
os << "consume %%" << OpArgs[0] << "\n";
os << "transfer %%" << OpArgs[0] << "\n";
break;
case PartitionOpKind::Merge:
os << "merge %%" << OpArgs[0] << " with %%" << OpArgs[1] << "\n";
Expand Down Expand Up @@ -200,8 +214,8 @@ static void horizontalUpdate(std::map<Element, Region> &map, Element key,
class Partition {
private:
// Label each index with a non-negative (unsigned) label if it is associated
// with a valid region, and with -1 if it is associated with a consumed region
// in-order traversal relied upon.
// with a valid region, and with -1 if it is associated with a transferred
// region in-order traversal relied upon.
std::map<Element, Region> labels;

// Track a label that is guaranteed to be strictly larger than all in use,
Expand All @@ -227,8 +241,8 @@ class Partition {
};

for (auto &[i, label] : labels) {
// correctness vacuous at consumed indices
if (label.isConsumed())
// correctness vacuous at transferred indices
if (label.isTransferred())
continue;

// this label should not exceed fresh_label
Expand All @@ -253,7 +267,7 @@ class Partition {

// linear time - For each region label that occurs, find the first index
// at which it occurs and relabel all instances of it to that index.
// This excludes the -1 label for consumed regions.
// This excludes the -1 label for transferred regions.
void canonicalize() {
if (canonical)
return;
Expand All @@ -263,8 +277,8 @@ class Partition {

// relies on in-order traversal of labels
for (auto &[i, label] : labels) {
// leave -1 (consumed region) as is
if (label.isConsumed())
// leave -1 (transferred region) as is
if (label.isTransferred())
continue;

if (!relabel.count(label)) {
Expand Down Expand Up @@ -309,7 +323,7 @@ class Partition {
// so set to false to begin with
Partition(bool canonical) : labels({}), canonical(canonical) {}

static Partition singleRegion(std::vector<Element> indices) {
static Partition singleRegion(ArrayRef<Element> indices) {
Partition p;
if (!indices.empty()) {
Region min_index =
Expand All @@ -335,8 +349,8 @@ class Partition {

bool isTracked(Element val) const { return labels.count(val); }

bool isConsumed(Element val) const {
return isTracked(val) && labels.at(val).isConsumed();
bool isTransferred(Element val) const {
return isTracked(val) && labels.at(val).isTransferred();
}

// quadratic time - Construct the partition corresponding to the join of the
Expand All @@ -361,9 +375,10 @@ class Partition {
// of indices that are in the same region in snd are also in the same region
// in fst - the desired property
for (const auto [i, snd_label] : snd_reduced.labels) {
if (snd_label.isConsumed())
// if snd says that the region has been consumed, mark it consumed in fst
horizontalUpdate(fst_reduced.labels, i, Region::consumed());
if (snd_label.isTransferred())
// if snd says that the region has been transferred, mark it transferred
// in fst
horizontalUpdate(fst_reduced.labels, i, Region::transferred());
else
fst_reduced.merge(i, Element(snd_label));
}
Expand All @@ -381,29 +396,35 @@ class Partition {

// Apply the passed PartitionOp to this partition, performing its action.
// A `handleFailure` closure can optionally be passed in that will be called
// if a consumed region is required. The closure is given the PartitionOp that
// failed, and the index of the SIL value that was required but consumed.
// Additionally, a list of "nonconsumable" indices can be passed in along with
// a handleConsumeNonConsumable closure. In the event that a region containing
// one of the nonconsumable indices is consumed, the closure will be called
// with the offending Consume.
// if a transferred region is required. The closure is given the PartitionOp
// that failed, and the index of the SIL value that was required but
// transferred. Additionally, a list of "nonconsumable" indices can be passed
// in along with a handleConsumeNonConsumable closure. In the event that a
// region containing one of the nonconsumable indices is transferred, the
// closure will be called with the offending Consume.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the offending Transfer?

void apply(
PartitionOp op,
llvm::function_ref<void(const PartitionOp &, Element)> handleFailure =
[](const PartitionOp &, Element) {},

ArrayRef<Element> nonconsumables = {},

llvm::function_ref<void(const PartitionOp &, Element)>
handleConsumeNonConsumable = [](const PartitionOp &, Element) {}) {
REGIONBASEDISOLATION_VERBOSE_LOG(llvm::dbgs() << "Applying: ";
op.print(llvm::dbgs()));
REGIONBASEDISOLATION_VERBOSE_LOG(llvm::dbgs() << " Before: ";
print(llvm::dbgs()));
SWIFT_DEFER {
REGIONBASEDISOLATION_VERBOSE_LOG(llvm::dbgs() << " After: ";
print(llvm::dbgs()));
};
switch (op.OpKind) {
case PartitionOpKind::Assign:
assert(op.OpArgs.size() == 2 &&
"Assign PartitionOp should be passed 2 arguments");
assert(labels.count(op.OpArgs[1]) &&
"Assign PartitionOp's source argument should be already tracked");
// if assigning to a missing region, handle the failure
if (isConsumed(op.OpArgs[1]))
if (isTransferred(op.OpArgs[1]))
handleFailure(op, op.OpArgs[1]);

labels.insert_or_assign(op.OpArgs[0], labels.at(op.OpArgs[1]));
Expand All @@ -429,25 +450,24 @@ class Partition {
assert(labels.count(op.OpArgs[0]) &&
"Consume PartitionOp's argument should already be tracked");

// check if any nonconsumables are consumed here, and handle the failure if so
// check if any nonconsumables are transferred here, and handle the
// failure if so
for (Element nonconsumable : nonconsumables) {
assert(labels.count(nonconsumable) &&
"nonconsumables should be function args and self, and therefore"
"always present in the label map because of initialization at "
"entry");
if (!isConsumed(nonconsumable) &&
if (!isTransferred(nonconsumable) &&
labels.at(nonconsumable) == labels.at(op.OpArgs[0])) {
handleConsumeNonConsumable(op, nonconsumable);
break;
}
}

// ensure region is consumed
if (!isConsumed(op.OpArgs[0]))
// mark region as consumed
horizontalUpdate(labels, op.OpArgs[0], Region::consumed());


// ensure region is transferred
if (!isTransferred(op.OpArgs[0]))
// mark region as transferred
horizontalUpdate(labels, op.OpArgs[0], Region::transferred());

break;
case PartitionOpKind::Merge:
Expand All @@ -456,10 +476,10 @@ class Partition {
assert(labels.count(op.OpArgs[0]) && labels.count(op.OpArgs[1]) &&
"Merge PartitionOp's arguments should already be tracked");

// if attempting to merge a consumed region, handle the failure
if (isConsumed(op.OpArgs[0]))
// if attempting to merge a transferred region, handle the failure
if (isTransferred(op.OpArgs[0]))
handleFailure(op, op.OpArgs[0]);
if (isConsumed(op.OpArgs[1]))
if (isTransferred(op.OpArgs[1]))
handleFailure(op, op.OpArgs[1]);

merge(op.OpArgs[0], op.OpArgs[1]);
Expand All @@ -469,26 +489,26 @@ class Partition {
"Require PartitionOp should be passed 1 argument");
assert(labels.count(op.OpArgs[0]) &&
"Require PartitionOp's argument should already be tracked");
if (isConsumed(op.OpArgs[0]))
if (isTransferred(op.OpArgs[0]))
handleFailure(op, op.OpArgs[0]);
}

assert(is_canonical_correct());
}

// return a vector of the consumed values in this partition
std::vector<Element> getConsumedVals() const {
// return a vector of the transferred values in this partition
std::vector<Element> getTransferredVals() const {
// for effeciency, this could return an iterator not a vector
std::vector<Element> consumedVals;
std::vector<Element> transferredVals;
for (auto [i, _] : labels)
if (isConsumed(i))
consumedVals.push_back(i);
return consumedVals;
if (isTransferred(i))
transferredVals.push_back(i);
return transferredVals;
}

// return a vector of the non-consumed regions in this partition, each
// return a vector of the non-transferred regions in this partition, each
// represented as a vector of values
std::vector<std::vector<Element>> getNonConsumedRegions() const {
std::vector<std::vector<Element>> getNonTransferredRegions() const {
// for effeciency, this could return an iterator not a vector
std::map<Region, std::vector<Element>> buckets;

Expand Down Expand Up @@ -523,12 +543,12 @@ class Partition {

os << "[";
for (auto [label, indices] : buckets) {
os << (label.isConsumed() ? "{" : "(");
os << (label.isTransferred() ? "{" : "(");
int j = 0;
for (Element i : indices) {
os << (j++ ? " " : "") << i;
}
os << (label.isConsumed() ? "}" : ")");
os << (label.isTransferred() ? "}" : ")");
}
os << "]\n";
}
Expand Down
Loading