Skip to content

Commit 95669c5

Browse files
gottesmmgottesmm
committed
[region-isolation] Instead of passing around an expression to get the original type, just derive the type from the transfer operand when we emit the error.
1 parent 957a79f commit 95669c5

File tree

2 files changed

+94
-66
lines changed

2 files changed

+94
-66
lines changed

‎include/swift/SILOptimizer/Utils/PartitionUtils.h

Lines changed: 12 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -124,32 +124,24 @@ class PartitionOp {
124124
/// generated during compilation from a SILBasicBlock
125125
PointerUnion<SILInstruction *, Operand *> source;
126126

127-
/// Record an AST expression corresponding to this PartitionOp, currently
128-
/// populated only for Transfer expressions to indicate the value being
129-
/// transferred
130-
Expr *sourceExpr;
131-
132127
// TODO: can the following declarations be merged?
133128
PartitionOp(PartitionOpKind opKind, Element arg1,
134-
SILInstruction *sourceInst = nullptr, Expr *sourceExpr = nullptr)
135-
: opKind(opKind), opArgs({arg1}), source(sourceInst),
136-
sourceExpr(sourceExpr) {
129+
SILInstruction *sourceInst = nullptr)
130+
: opKind(opKind), opArgs({arg1}), source(sourceInst) {
137131
assert((opKind != PartitionOpKind::Transfer || sourceInst) &&
138132
"Transfer needs a sourceInst");
139133
}
140134

141-
PartitionOp(PartitionOpKind opKind, Element arg1,
142-
Operand *sourceOperand, Expr *sourceExpr = nullptr)
143-
: opKind(opKind), opArgs({arg1}), source(sourceOperand), sourceExpr(sourceExpr) {
144-
assert((opKind != PartitionOpKind::Transfer || source) &&
135+
PartitionOp(PartitionOpKind opKind, Element arg1, Operand *sourceOperand)
136+
: opKind(opKind), opArgs({arg1}), source(sourceOperand) {
137+
assert((opKind != PartitionOpKind::Transfer || sourceOperand) &&
145138
"Transfer needs a sourceInst");
146139
}
147140

148-
PartitionOp(PartitionOpKind OpKind, Element arg1, Element arg2,
149-
SILInstruction *sourceInst = nullptr, Expr *sourceExpr = nullptr)
150-
: opKind(OpKind), opArgs({arg1, arg2}), source(sourceInst),
151-
sourceExpr(sourceExpr) {
152-
assert((OpKind != PartitionOpKind::Transfer || sourceInst) &&
141+
PartitionOp(PartitionOpKind opKind, Element arg1, Element arg2,
142+
SILInstruction *sourceInst = nullptr)
143+
: opKind(opKind), opArgs({arg1, arg2}), source(sourceInst) {
144+
assert((opKind != PartitionOpKind::Transfer || sourceInst) &&
153145
"Transfer needs a sourceInst");
154146
}
155147

@@ -166,10 +158,8 @@ class PartitionOp {
166158
return PartitionOp(PartitionOpKind::AssignFresh, tgt, sourceInst);
167159
}
168160

169-
static PartitionOp Transfer(Element tgt, Operand *transferringOp,
170-
Expr *sourceExpr = nullptr) {
171-
return PartitionOp(PartitionOpKind::Transfer, tgt, transferringOp,
172-
sourceExpr);
161+
static PartitionOp Transfer(Element tgt, Operand *transferringOp) {
162+
return PartitionOp(PartitionOpKind::Transfer, tgt, transferringOp);
173163
}
174164

175165
static PartitionOp Merge(Element tgt1, Element tgt2,
@@ -207,11 +197,7 @@ class PartitionOp {
207197

208198
Operand *getSourceOp() const { return source.get<Operand *>(); }
209199

210-
SILLocation getSourceLoc() const { return getSourceInst(true)->getLoc(); }
211-
212-
Expr *getSourceExpr() const {
213-
return sourceExpr;
214-
}
200+
SILLocation getSourceLoc() const { return getSourceInst()->getLoc(); }
215201

216202
SWIFT_DEBUG_DUMP { print(llvm::dbgs()); }
217203

‎lib/SILOptimizer/Mandatory/TransferNonSendable.cpp

Lines changed: 82 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,64 @@ static bool isProjectedFromAggregate(SILValue value) {
223223
return visitor.isMerge;
224224
}
225225

226+
//===----------------------------------------------------------------------===//
227+
// MARK: Expr/Type Inference for Diagnostics
228+
//===----------------------------------------------------------------------===//
229+
230+
namespace {
231+
232+
struct InferredCallerArgumentTypeInfo {
233+
Type inferredType;
234+
ApplyExpr *sourceApply;
235+
ApplyIsolationCrossing isolationCrossing;
236+
Expr *foundExpr;
237+
238+
InferredCallerArgumentTypeInfo(const Operand *op);
239+
};
240+
241+
} // namespace
242+
243+
InferredCallerArgumentTypeInfo::InferredCallerArgumentTypeInfo(
244+
const Operand *op)
245+
: inferredType(op->get()->getType().getASTType()), sourceApply(nullptr),
246+
isolationCrossing(), foundExpr(nullptr) {
247+
sourceApply = op->getUser()->getLoc().getAsASTNode<ApplyExpr>();
248+
assert(sourceApply);
249+
isolationCrossing = *sourceApply->getIsolationCrossing();
250+
251+
// Grab out full apply site and see if we can find a better expr.
252+
SILInstruction *i = const_cast<SILInstruction *>(op->getUser());
253+
auto fai = FullApplySite::isa(i);
254+
255+
// If we have self, then infer it.
256+
if (fai.hasSelfArgument() && op == &fai.getSelfArgumentOperand()) {
257+
if (auto callExpr = dyn_cast<CallExpr>(sourceApply))
258+
if (auto calledExpr =
259+
dyn_cast<DotSyntaxCallExpr>(callExpr->getDirectCallee()))
260+
foundExpr = calledExpr->getBase();
261+
} else {
262+
// Otherwise, try to infer using the operand of the ApplyExpr.
263+
unsigned argNum = [&]() -> unsigned {
264+
if (fai.isCalleeOperand(*op))
265+
return op->getOperandNumber();
266+
return fai.getAppliedArgIndex(*op);
267+
}();
268+
if (argNum < sourceApply->getArgs()->size()) {
269+
auto *expr = sourceApply->getArgs()->getExpr(argNum);
270+
271+
// If we have an erasure expression, lets use the original type. We do
272+
// this since we are not saying the specific parameter that is the
273+
// issue and we are using the type to explain it to the user.
274+
if (auto *erasureExpr = dyn_cast<ErasureExpr>(expr))
275+
expr = erasureExpr->getSubExpr();
276+
foundExpr = expr;
277+
}
278+
}
279+
280+
if (foundExpr)
281+
inferredType = foundExpr->findOriginalType();
282+
}
283+
226284
//===----------------------------------------------------------------------===//
227285
// MARK: Instruction Level Model
228286
//===----------------------------------------------------------------------===//
@@ -391,12 +449,12 @@ struct PartitionOpBuilder {
391449
lookupValueID(tgt), lookupValueID(src), currentInst));
392450
}
393451

394-
void addTransfer(SILValue representative, Operand *op, Expr *expr) {
452+
void addTransfer(SILValue representative, Operand *op) {
395453
assert(valueHasID(representative) &&
396454
"transferred value should already have been encountered");
397455

398456
currentInstPartitionOps.emplace_back(
399-
PartitionOp::Transfer(lookupValueID(representative), op, expr));
457+
PartitionOp::Transfer(lookupValueID(representative), op));
400458
}
401459

402460
void addMerge(SILValue fst, SILValue snd) {
@@ -832,39 +890,23 @@ class PartitionOpTranslator {
832890
if (auto value = tryToTrackValue(op))
833891
builder.addRequire(value->getRepresentative());
834892

835-
auto getSourceArg = [&](unsigned i) {
836-
if (i < sourceApply->getArgs()->size())
837-
return sourceApply->getArgs()->getExpr(i);
838-
return (Expr *)nullptr;
839-
};
840-
841-
auto getSourceSelf = [&]() {
842-
if (auto callExpr = dyn_cast<CallExpr>(sourceApply))
843-
if (auto calledExpr =
844-
dyn_cast<DotSyntaxCallExpr>(callExpr->getDirectCallee()))
845-
return calledExpr->getBase();
846-
return (Expr *)nullptr;
847-
};
848-
849893
auto handleSILOperands = [&](MutableArrayRef<Operand> operands) {
850-
int argNum = 0;
851-
for (auto &operand : operands) {
852-
if (auto value = tryToTrackValue(operand.get()))
853-
builder.addTransfer(value->getRepresentative(), &operand, getSourceArg(argNum));
854-
argNum++;
894+
for (auto &op : operands) {
895+
if (auto value = tryToTrackValue(op.get()))
896+
builder.addTransfer(value->getRepresentative(), &op);
855897
}
856898
};
857899

858-
auto handleSILSelf = [&](Operand &self) {
859-
if (auto value = tryToTrackValue(self.get()))
860-
builder.addTransfer(value->getRepresentative(), &self, getSourceSelf());
900+
auto handleSILSelf = [&](Operand *self) {
901+
if (auto value = tryToTrackValue(self->get()))
902+
builder.addTransfer(value->getRepresentative(), self);
861903
};
862904

863905
if (applySite.hasSelfArgument()) {
864906
handleSILOperands(applySite.getOperandsWithoutSelf());
865-
handleSILSelf(applySite.getSelfArgumentOperand());
907+
handleSILSelf(&applySite.getSelfArgumentOperand());
866908
} else {
867-
handleSILOperands(applySite.getArgumentOperands());
909+
handleSILOperands(applySite->getAllOperands());
868910
}
869911

870912
// non-sendable results can't be returned from cross-isolation calls without
@@ -1660,8 +1702,8 @@ class TransferRequireAccumulator {
16601702
auto diag = fn->getASTContext().Diags.diagnose(
16611703
loc.getSourceLoc(), diag::transfer_yields_race, numDisplayed,
16621704
numDisplayed != 1, numHidden > 0, numHidden);
1663-
if (auto sourceExpr = transferOp.getSourceExpr())
1664-
diag.highlight(sourceExpr->getSourceRange());
1705+
if (auto sourceExpr = transferOp.getSourceLoc())
1706+
diag.highlight(sourceExpr.getSourceRange());
16651707
return;
16661708
}
16671709

@@ -1671,7 +1713,7 @@ class TransferRequireAccumulator {
16711713
// transfer...
16721714
if (numRequiresToProcess-- == 0)
16731715
break;
1674-
auto loc = requireOp.getSourceLoc();
1716+
auto loc = requireOp.getSourceInst()->getLoc();
16751717
fn->getASTContext()
16761718
.Diags.diagnose(loc.getSourceLoc(), diag::possible_racy_access_site)
16771719
.highlight(loc.getSourceRange());
@@ -1710,17 +1752,17 @@ class TransferRequireAccumulator {
17101752
assert(isolationCrossing && "ApplyExprs should be transferring only if "
17111753
"they are isolation crossing");
17121754

1713-
auto argExpr = transferOp.getSourceExpr();
1714-
assert(argExpr && "sourceExpr should be populated for ApplyExpr transfers");
1755+
InferredCallerArgumentTypeInfo argTypeInfo(transferOp.getSourceOp());
17151756

1757+
auto loc = transferOp.getSourceLoc();
17161758
sourceInst->getFunction()
17171759
->getASTContext()
17181760
.Diags
1719-
.diagnose(argExpr->getLoc(), diag::call_site_transfer_yields_race,
1720-
argExpr->findOriginalType(),
1721-
isolationCrossing.value().getCallerIsolation(),
1722-
isolationCrossing.value().getCalleeIsolation())
1723-
.highlight(argExpr->getSourceRange());
1761+
.diagnose(loc.getSourceLoc(), diag::call_site_transfer_yields_race,
1762+
argTypeInfo.inferredType,
1763+
argTypeInfo.isolationCrossing.getCallerIsolation(),
1764+
argTypeInfo.isolationCrossing.getCalleeIsolation())
1765+
.highlight(loc.getSourceRange());
17241766
return true;
17251767
}
17261768
};
@@ -2206,17 +2248,17 @@ class PartitionAnalysis {
22062248
" they are isolation crossing");
22072249
return false;
22082250
}
2209-
auto argExpr = transferOp.getSourceExpr();
2251+
auto argExpr = transferOp.getSourceLoc();
22102252
if (!argExpr)
22112253
assert(false && "sourceExpr should be populated for ApplyExpr transfers");
22122254

22132255
function->getASTContext()
22142256
.Diags
2215-
.diagnose(argExpr->getLoc(), diag::call_site_transfer_yields_race,
2216-
argExpr->findOriginalType(),
2257+
.diagnose(argExpr.getSourceLoc(), diag::call_site_transfer_yields_race,
2258+
transferOp.getSourceOp()->get()->getType().getASTType(),
22172259
isolationCrossing.value().getCallerIsolation(),
22182260
isolationCrossing.value().getCalleeIsolation())
2219-
.highlight(argExpr->getSourceRange());
2261+
.highlight(argExpr.getSourceRange());
22202262
return true;
22212263
}
22222264

0 commit comments

Comments
 (0)