Skip to content

Commit 7c6d54e

Browse files
committed
OpaqueValues: fix emitDistributedActorSystemWitnessCall
This code was not consulting SILFunctionConventions, so it was passing things indirect when they are not in sil-opaque-values. This fixes callers as well to ensure they pass arguments and results correctly in both modes.
1 parent 199156b commit 7c6d54e

File tree

6 files changed

+136
-88
lines changed

6 files changed

+136
-88
lines changed

include/swift/SILOptimizer/Utils/DistributedActor.h

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
#include "swift/AST/Decl.h"
1717
#include "llvm/ADT/ArrayRef.h"
18+
#include "swift/SIL/SILValue.h"
1819
#include <optional>
1920
#include <utility>
2021

@@ -30,7 +31,6 @@ class SILArgument;
3031
class SILFunction;
3132
class SILLocation;
3233
class SILType;
33-
class SILValue;
3434

3535
/// Creates a reference to the distributed actor's \p actorSystem
3636
/// stored property.
@@ -46,11 +46,16 @@ SILValue refDistributedActorSystem(SILBuilder &B,
4646
/// \param actorType If non-empty, the type of the distributed actor that is
4747
/// provided as one of the arguments.
4848
/// \param args The arguments provided to the call, not including the base.
49+
/// \param indirectResult If the result is known to be returned indirect,
50+
/// this is the temporary storage for it.
4951
/// \param tryTargets For a call that can throw, the normal and error basic
5052
/// blocks that the call will branch to.
51-
void emitDistributedActorSystemWitnessCall(
53+
/// \returns If the apply result is known to be returned directly,
54+
/// and there are no tryTargets, then the result is returned.
55+
std::optional<SILValue> emitDistributedActorSystemWitnessCall(
5256
SILBuilder &B, SILLocation loc, DeclName methodName, SILValue base,
5357
SILType actorType, llvm::ArrayRef<SILValue> args,
58+
std::optional<SILValue> indirectResult = std::nullopt,
5459
std::optional<std::pair<SILBasicBlock *, SILBasicBlock *>> tryTargets =
5560
std::nullopt);
5661

lib/SILGen/SILGenDistributed.cpp

Lines changed: 38 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,17 @@ static void initializeProperty(SILGenFunction &SGF, SILLocation loc,
6363

6464
auto fieldAddr = emitActorPropertyReference(SGF, loc, actorSelf, prop);
6565

66-
if (loweredType.isAddressOnly(SGF.F)) {
66+
if (loweredType.isAddressOnly(SGF.F) && SGF.useLoweredAddresses()) {
6767
SGF.B.createCopyAddr(loc, value, fieldAddr, isTake, IsInitialization);
6868
} else {
6969
if (value->getType().isAddress()) {
7070
SGF.emitSemanticLoadInto(loc, value, SGF.F.getTypeLowering(value->getType()),
7171
fieldAddr, SGF.getTypeLowering(loweredType), isTake, IsInitialization);
7272
} else {
73-
value = SGF.B.emitCopyValueOperation(loc, value);
73+
// If it's not semantically a take, copy it.
74+
if (isTake == IsNotTake)
75+
value = SGF.B.emitCopyValueOperation(loc, value);
76+
7477
SGF.B.emitStoreValueOperation(
7578
loc, value, fieldAddr, StoreOwnershipQualifier::Init);
7679
}
@@ -204,20 +207,31 @@ void SILGenFunction::emitDistActorIdentityInit(ConstructorDecl *ctor,
204207
auto selfMetatype = getLoweredType(MetatypeType::get(selfTy));
205208
SILValue selfMetatypeValue = B.createMetatype(loc, selfMetatype);
206209

207-
// --- create a temporary storage for the result of the call
208-
// it will be deallocated automatically as we exit this scope
209210
VarDecl *var = classDecl->getDistributedActorIDProperty();
210-
auto resultTy = getLoweredType(F.mapTypeIntoEnvironment(var->getInterfaceType()));
211-
auto temp = emitTemporaryAllocation(loc, resultTy);
212-
213-
// --- emit the call itself.
214-
emitDistributedActorSystemWitnessCall(
215-
B, loc, C.Id_assignID,
216-
actorSystem, getLoweredType(selfTy),
217-
{ temp, selfMetatypeValue });
218-
219-
// --- initialize the property.
220-
initializeProperty(*this, loc, borrowedSelfArg, var, temp, IsTake);
211+
if (useLoweredAddresses()) {
212+
// --- create a temporary storage for the result of the call
213+
// it will be deallocated automatically as we exit this scope
214+
auto resultTy = getLoweredType(F.mapTypeIntoEnvironment(var->getInterfaceType()));
215+
auto temp = emitTemporaryAllocation(loc, resultTy);
216+
217+
// --- emit the call itself.
218+
emitDistributedActorSystemWitnessCall(
219+
B, loc, C.Id_assignID,
220+
actorSystem, getLoweredType(selfTy),
221+
{ selfMetatypeValue }, temp);
222+
223+
// --- initialize the property.
224+
initializeProperty(*this, loc, borrowedSelfArg, var, temp, IsTake);
225+
} else {
226+
// --- emit the call itself.
227+
auto result = emitDistributedActorSystemWitnessCall(
228+
B, loc, C.Id_assignID,
229+
actorSystem, getLoweredType(selfTy),
230+
{ selfMetatypeValue });
231+
232+
// --- initialize the property.
233+
initializeProperty(*this, loc, borrowedSelfArg, var, result.value(), IsTake);
234+
}
221235
}
222236

223237
// TODO(distributed): rename to DistributedActorID
@@ -361,27 +375,6 @@ void SILGenFunction::emitDistributedActorReady(
361375
// ==== ------------------------------------------------------------------------
362376
// MARK: remote instance initialization
363377

364-
/// emit a call to the distributed actor system's resolve function:
365-
///
366-
/// \verbatim
367-
/// system.resolve(id:as:)
368-
/// \endverbatim
369-
static void createDistributedActorFactory_resolve(
370-
SILGenFunction &SGF, ASTContext &C, FuncDecl *fd, SILValue idValue,
371-
SILValue actorSystemValue, Type selfTy, SILValue selfMetatypeValue,
372-
SILType resultTy, SILBasicBlock *normalBB, SILBasicBlock *errorBB) {
373-
auto &B = SGF.B;
374-
375-
auto loc = SILLocation(fd);
376-
loc.markAutoGenerated();
377-
378-
// // ---- actually call system.resolve(id: id, as: Self.self)
379-
emitDistributedActorSystemWitnessCall(
380-
B, loc, C.Id_resolve, actorSystemValue, SGF.getLoweredType(selfTy),
381-
{ idValue, selfMetatypeValue },
382-
std::make_pair(normalBB, errorBB));
383-
}
384-
385378
/// Function body of:
386379
/// \verbatim
387380
/// DistributedActor.resolve(
@@ -435,9 +428,15 @@ void SILGenFunction::emitDistributedActorFactory(FuncDecl *fd) { // TODO(distrib
435428

436429
// ==== Call `try system.resolve(id: id, as: Self.self)`
437430
{
438-
createDistributedActorFactory_resolve(
439-
*this, C, fd, idArg, actorSystemArg, selfTy, selfMetatypeValue,
440-
optionalReturnTy, switchBB, errorBB);
431+
auto loc = SILLocation(fd);
432+
loc.markAutoGenerated();
433+
434+
// // ---- actually call system.resolve(id: id, as: Self.self)
435+
emitDistributedActorSystemWitnessCall(
436+
B, loc, C.Id_resolve, actorSystemArg, getLoweredType(selfTy),
437+
{ idArg, selfMetatypeValue },
438+
/*indirectResult=*/std::nullopt,
439+
std::make_pair(switchBB, errorBB));
441440
}
442441

443442
// ==== switch resolved { ... }

lib/SILOptimizer/Utils/DistributedActor.cpp

Lines changed: 88 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,15 @@
2020

2121
namespace swift {
2222

23-
void emitDistributedActorSystemWitnessCall(
23+
/// \returns the result of the call, if returned directly.
24+
std::optional<SILValue> emitDistributedActorSystemWitnessCall(
2425
SILBuilder &B, SILLocation loc, DeclName methodName, SILValue base,
2526
// types to be passed through to SubstitutionMap:
2627
SILType actorType,
2728
// call arguments, except the base which will be passed last
2829
ArrayRef<SILValue> args,
30+
// pre-allocated, uninitialized indirect result storage, if needed
31+
std::optional<SILValue> indirectResult,
2932
std::optional<std::pair<SILBasicBlock *, SILBasicBlock *>> tryTargets) {
3033
auto &F = B.getFunction();
3134
auto &M = B.getModule();
@@ -73,7 +76,6 @@ void emitDistributedActorSystemWitnessCall(
7376
KnownProtocolKind::DistributedActor);
7477
assert(actorProto);
7578

76-
ProtocolConformanceRef conformance;
7779
auto distributedActorConfRef = lookupConformance(
7880
actorType.getASTType(), actorProto);
7981
assert(!distributedActorConfRef.isInvalid() &&
@@ -85,42 +87,61 @@ void emitDistributedActorSystemWitnessCall(
8587
subs = SubstitutionMap::get(genericSig, subTypes, subConformances);
8688
}
8789

88-
std::optional<SILValue> temporaryArgumentBuffer;
89-
90-
// If the self parameter is indirect but the base is a value, put it
91-
// into a temporary allocation.
9290
auto methodSILFnTy = methodSILTy.castTo<SILFunctionType>();
93-
std::optional<SILValue> temporaryActorSystemBuffer;
94-
if (methodSILFnTy->getSelfParameter().isFormalIndirect() &&
95-
!base->getType().isAddress()) {
96-
auto buf = B.createAllocStack(loc, base->getType(), std::nullopt);
97-
base = B.emitCopyValueOperation(loc, base);
98-
B.emitStoreValueOperation(
99-
loc, base, buf, StoreOwnershipQualifier::Init);
100-
temporaryActorSystemBuffer = SILValue(buf);
101-
}
91+
SILFunctionConventions conv(methodSILFnTy, M);
92+
93+
// Since this code lives outside of SILGen, manage our clean-ups manually.
94+
SmallVector<SILInstruction *, 2> cleanups;
95+
96+
auto prepareArgument = [&](SILParameterInfo param, SILValue arg) -> SILValue {
97+
if (conv.isSILIndirect(param)) {
98+
// Does it need temporary stack storage?
99+
if (!arg->getType().isAddress() &&
100+
!dyn_cast<AnyMetatypeType>(arg->getType().getASTType())) {
101+
auto buf = B.createAllocStack(loc, arg->getType(), std::nullopt);
102+
cleanups.push_back(buf);
103+
104+
auto copy = B.emitCopyValueOperation(loc, arg);
105+
B.emitStoreValueOperation(
106+
loc, copy, buf, StoreOwnershipQualifier::Init);
107+
108+
return buf;
109+
}
110+
return arg; // no temporary storage needed
111+
}
112+
113+
// Otherwise, it's a direct convention. Borrow if needed.
114+
if (arg->getType().isAddress()) {
115+
arg = B.emitLoadBorrowOperation(loc, arg);
116+
cleanups.push_back(arg.getDefiningInstruction());
117+
}
118+
return arg;
119+
};
120+
121+
SILValue selfArg = prepareArgument(methodSILFnTy->getSelfParameter(), base);
102122

103123
// === Call the method.
104124
// --- Push the arguments
105125
SmallVector<SILValue, 2> allArgs;
126+
127+
const bool hasIndirectResult = conv.getNumIndirectSILResults() > 0;
128+
ASSERT(hasIndirectResult == indirectResult.has_value() && "no indirectResult storage given!");
129+
ASSERT(conv.getNumIndirectSILResults() <= 1);
130+
131+
const bool hasDirectResult = conv.getNumDirectSILResults() > 0;
132+
ASSERT(!(hasIndirectResult && hasDirectResult) && "indirect AND direct results aren't supported");
133+
ASSERT(conv.getNumDirectSILResults() <= 1);
134+
135+
if (hasIndirectResult) {
136+
allArgs.push_back(*indirectResult);
137+
}
138+
106139
auto params = methodSILFnTy->getParameters();
107140
for (size_t i = 0; i < args.size(); ++i) {
108-
auto arg = args[i];
109-
if (params[i].isFormalIndirect() &&
110-
!arg->getType().isAddress() &&
111-
!dyn_cast<AnyMetatypeType>(arg->getType().getASTType())) {
112-
auto buf = B.createAllocStack(loc, arg->getType(), std::nullopt);
113-
auto argCopy = B.emitCopyValueOperation(loc, arg);
114-
B.emitStoreValueOperation(
115-
loc, argCopy, buf, StoreOwnershipQualifier::Init);
116-
temporaryArgumentBuffer = SILValue(buf);
117-
allArgs.push_back(*temporaryArgumentBuffer);
118-
} else {
119-
allArgs.push_back(arg);
120-
}
141+
allArgs.push_back(prepareArgument(params[i], args[i]));
121142
}
143+
122144
// Push the self argument
123-
auto selfArg = temporaryActorSystemBuffer ? *temporaryActorSystemBuffer : base;
124145
allArgs.push_back(selfArg);
125146

126147
SILInstruction *apply;
@@ -132,7 +153,7 @@ void emitDistributedActorSystemWitnessCall(
132153
apply = B.createApply(loc, witnessMethod, subs, allArgs);
133154
}
134155

135-
// Local function to emit a cleanup after the call.
156+
// Local function to emit cleanups after the call in successor blocks.
136157
auto emitCleanup = [&](llvm::function_ref<void(SILBuilder &builder)> fn) {
137158
if (tryTargets) {
138159
{
@@ -148,25 +169,45 @@ void emitDistributedActorSystemWitnessCall(
148169
}
149170
};
150171

151-
// ==== If we had to create a buffers we need to clean them up
152-
// --- Cleanup id buffer
153-
if (temporaryArgumentBuffer) {
154-
emitCleanup([&](SILBuilder & builder) {
155-
auto value = builder.emitLoadValueOperation(
156-
loc, *temporaryArgumentBuffer, LoadOwnershipQualifier::Take);
157-
builder.emitDestroyValueOperation(loc, value);
158-
builder.createDeallocStack(loc, *temporaryArgumentBuffer);
159-
});
172+
// Emit clean-ups in reverse order, to preserve stack nesting, etc.
173+
for (auto inst : reverse(cleanups)) {
174+
if (auto asi = dyn_cast<AllocStackInst>(inst)) {
175+
auto buf = asi->getResult(0);
176+
emitCleanup([&](SILBuilder & builder) {
177+
// FIXME: could do destroy_addr rather than take + destroy_value
178+
auto value = builder.emitLoadValueOperation(
179+
loc, buf, LoadOwnershipQualifier::Take);
180+
builder.emitDestroyValueOperation(loc, value);
181+
builder.createDeallocStack(loc, buf);
182+
});
183+
continue;
184+
}
185+
186+
if (auto lb = dyn_cast<LoadBorrowInst>(inst)) {
187+
auto borrow = lb->getResult(0);
188+
emitCleanup([&](SILBuilder & builder) {
189+
builder.emitEndBorrowOperation(loc, borrow);
190+
});
191+
continue;
192+
}
193+
194+
if (isa<LoadInst>(inst)) {
195+
// no clean-ups required
196+
continue;
197+
}
198+
199+
llvm_unreachable("unknown instruction kind to clean-up!");
160200
}
161-
// --- Cleanup base buffer
162-
if (temporaryActorSystemBuffer) {
163-
emitCleanup([&](SILBuilder & builder) {
164-
auto value = builder.emitLoadValueOperation(
165-
loc, *temporaryActorSystemBuffer, LoadOwnershipQualifier::Take);
166-
builder.emitDestroyValueOperation(loc, value);
167-
builder.createDeallocStack(loc, *temporaryActorSystemBuffer);
168-
});
201+
202+
// If this was a try_apply, then the result is the BB argument of the
203+
// successor block. We let our caller figure that out themselves.
204+
//
205+
// Otherwise, the apply had a single direct result, so we return that.
206+
if (hasDirectResult && !tryTargets) {
207+
return apply->getResult(0);
169208
}
209+
210+
return std::nullopt;
170211
}
171212

172213
void emitActorReadyCall(SILBuilder &B, SILLocation loc, SILValue actor,

test/Distributed/Runtime/distributed_actor_decode.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// RUN: %target-run-simple-swift( -target %target-swift-5.7-abi-triple -parse-as-library) | %FileCheck %s
2+
// RUN: %target-run-simple-swift(-Xfrontend -enable-sil-opaque-values -target %target-swift-5.7-abi-triple -parse-as-library) | %FileCheck %s
23

34
// REQUIRES: executable_test
45
// REQUIRES: concurrency

test/Distributed/Runtime/distributed_actor_init_local.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// RUN: %target-run-simple-swift( -target %target-swift-5.7-abi-triple -parse-as-library) | %FileCheck %s
2+
// RUN: %target-run-simple-swift(-Xfrontend -enable-sil-opaque-values -target %target-swift-5.7-abi-triple -parse-as-library) | %FileCheck %s
23

34
// REQUIRES: executable_test
45
// REQUIRES: concurrency

test/Distributed/Runtime/distributed_actor_remote_functions.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
// RUN: %target-run-simple-swift(-target %target-swift-5.7-abi-triple -parse-as-library) | %FileCheck %s
2+
// RUN: %target-run-simple-swift(-Xfrontend -enable-sil-opaque-values -target %target-swift-5.7-abi-triple -parse-as-library) | %FileCheck %s
23

34
// REQUIRES: executable_test
45
// REQUIRES: concurrency

0 commit comments

Comments
 (0)