Skip to content

Commit 48132ea

Browse files
NobodyNadaNobodyNada
committed
[SILOptimizer] address key path projection review feedback
1 parent e1ceb4f commit 48132ea

File tree

2 files changed

+273
-150
lines changed

2 files changed

+273
-150
lines changed

‎lib/SILOptimizer/Utils/KeyPathProjector.cpp

Lines changed: 45 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -104,13 +104,13 @@ class StoredPropertyProjector : public ComponentProjector {
104104
else
105105
parentAccessType = AccessType::Modify;
106106

107-
parent->project(parentAccessType, [&](SILValue parent) {
108-
callback(builder.createStructElementAddr(loc, parent, storedProperty));
107+
parent->project(parentAccessType, [&](SILValue parentValue) {
108+
callback(builder.createStructElementAddr(loc, parentValue, storedProperty));
109109
});
110110
} else {
111111
// Accessing a class member -> reading the class
112-
parent->project(AccessType::Get, [&](SILValue parent) {
113-
SingleValueInstruction *Ref = builder.createLoad(loc, parent,
112+
parent->project(AccessType::Get, [&](SILValue parentValue) {
113+
SingleValueInstruction *Ref = builder.createLoad(loc, parentValue,
114114
LoadOwnershipQualifier::Unqualified);
115115

116116
// If we were previously accessing a class member, we're done now.
@@ -149,7 +149,6 @@ class StoredPropertyProjector : public ComponentProjector {
149149
// end the access now
150150
if (beginAccess == addr) {
151151
insertEndAccess(beginAccess, builder);
152-
beginAccess = nullptr;
153152
}
154153
});
155154
}
@@ -178,8 +177,8 @@ class TupleElementProjector : public ComponentProjector {
178177
else
179178
parentAccessType = AccessType::Modify;
180179

181-
parent->project(parentAccessType, [&](SILValue parent) {
182-
callback(builder.createTupleElementAddr(loc, parent,
180+
parent->project(parentAccessType, [&](SILValue parentValue) {
181+
callback(builder.createTupleElementAddr(loc, parentValue,
183182
component.getTupleIndex()));
184183
});
185184
}
@@ -203,7 +202,7 @@ class GettablePropertyProjector : public ComponentProjector {
203202
KeyPathPatternComponent::Kind::SettableProperty);
204203
assert(accessType == AccessType::Get && "property is not settable");
205204

206-
parent->project(accessType, [&](SILValue parent) {
205+
parent->project(accessType, [&](SILValue parentValue) {
207206
auto getter = component.getComputedPropertyGetter();
208207

209208
// The callback expects a memory address it can read from,
@@ -212,36 +211,17 @@ class GettablePropertyProjector : public ComponentProjector {
212211
SILType type = function.getLoweredType(component.getComponentType());
213212
auto addr = builder.createAllocStack(loc, type);
214213

215-
// Get subscript indices.
216-
auto context = createContextContainer();
217-
SILValue contextPtr = SILValue();
218-
if (context) {
219-
auto rawPtrType = builder.getASTContext().TheRawPointerType;
220-
auto rawPtrLoweredType = function.getLoweredLoadableType(rawPtrType);
221-
auto rawPtr = builder.createAddressToPointer(loc, context, rawPtrLoweredType);
222-
223-
auto ptrDecl = builder.getASTContext().getUnsafeRawPointerDecl();
224-
SILType ptrType = function.getLoweredType(ptrDecl->getDeclaredType());
225-
contextPtr = builder.createStruct(loc, ptrType, {rawPtr});
226-
}
214+
assertHasNoContext();
215+
assert(getter->getArguments().size() == 2);
227216

228217
auto ref = builder.createFunctionRef(loc, getter);
229-
if (contextPtr)
230-
builder.createApply(loc, ref, subs, {addr, parent, contextPtr});
231-
else
232-
builder.createApply(loc, ref, subs, {addr, parent});
218+
builder.createApply(loc, ref, subs, {addr, parentValue});
233219

234220
// If we were previously accessing a class member, we're done now.
235221
insertEndAccess(beginAccess, builder);
236-
beginAccess = nullptr;
237222

238223
callback(addr);
239224

240-
if (context) {
241-
builder.createDestroyAddr(loc, context);
242-
builder.createDeallocStack(loc, context);
243-
}
244-
245225
builder.createDestroyAddr(loc, addr);
246226
builder.createDeallocStack(loc, addr);
247227
});
@@ -252,52 +232,11 @@ class GettablePropertyProjector : public ComponentProjector {
252232
SubstitutionMap subs;
253233
BeginAccessInst *&beginAccess;
254234

255-
// Creates the subscript index context for a computed property.
256-
// Returns either null or a stack address that must be deallocated.
257-
SILValue createContextContainer() {
258-
auto indices = component.getSubscriptIndices();
259-
if (indices.empty()) return SILValue();
260-
261-
if (indices.size() == 1) {
262-
auto index = indices.front();
263-
auto addr = builder.createAllocStack(loc, index.LoweredType);
264-
auto value = keyPath->getAllOperands()[index.Operand].get();
265-
if (value->getType().isObject())
266-
builder.createStore(loc, value, addr,
267-
StoreOwnershipQualifier::Unqualified);
268-
else
269-
builder.createCopyAddr(loc, value, addr,
270-
IsNotTake, IsInitialization);
271-
return addr;
272-
} else {
273-
// Create a tuple containing the context.
274-
std::vector<TupleTypeElt> elementTypes;
275-
elementTypes.reserve(indices.size());
276-
for (auto index : indices) {
277-
elementTypes.push_back(TupleTypeElt(index.FormalType));
278-
}
279-
auto tupleType = TupleType::get(ArrayRef<TupleTypeElt>(elementTypes),
280-
builder.getASTContext());
281-
282-
auto &function = builder.getFunction();
283-
SILType lowered = function.getLoweredType(tupleType);
284-
285-
auto tupleAddr = builder.createAllocStack(loc, lowered);
286-
287-
// Copy the context elements into the tuple.
288-
for (size_t i = 0; i < indices.size(); i++) {
289-
auto index = indices[i];
290-
auto elementAddr = builder.createTupleElementAddr(loc, tupleAddr, i);
291-
auto value = keyPath->getAllOperands()[index.Operand].get();
292-
if (value->getType().isObject())
293-
builder.createStore(loc, value, elementAddr,
294-
StoreOwnershipQualifier::Unqualified);
295-
else
296-
builder.createCopyAddr(loc, value, elementAddr,
297-
IsNotTake, IsInitialization);
298-
}
299-
return tupleAddr;
300-
}
235+
void assertHasNoContext() {
236+
assert(component.getSubscriptIndices().empty() &&
237+
component.getExternalSubstitutions().empty() &&
238+
"cannot yet optimize key path component with external context; "
239+
"we should have checked for this before trying to project");
301240
}
302241
};
303242

@@ -330,10 +269,14 @@ class SettablePropertyProjector : public GettablePropertyProjector {
330269
if (component.isComputedSettablePropertyMutating()) {
331270
// A mutating setter modifies the parent
332271
parentAccessType = AccessType::Modify;
333-
if (beginAccess) beginAccess->setAccessKind(SILAccessKind::Modify);
334-
} else parentAccessType = AccessType::Get;
272+
if (beginAccess) {
273+
beginAccess->setAccessKind(SILAccessKind::Modify);
274+
}
275+
} else {
276+
parentAccessType = AccessType::Get;
277+
}
335278

336-
parent->project(parentAccessType, [&](SILValue parent) {
279+
parent->project(parentAccessType, [&](SILValue parentValue) {
337280
auto getter = component.getComputedPropertyGetter();
338281
auto setter = component.getComputedPropertySetter();
339282

@@ -342,44 +285,24 @@ class SettablePropertyProjector : public GettablePropertyProjector {
342285
auto &function = builder.getFunction();
343286
SILType type = function.getLoweredType(component.getComponentType());
344287
auto addr = builder.createAllocStack(loc, type);
345-
346-
// Get subscript indices.
347-
auto context = createContextContainer();
348-
SILValue contextPtr = SILValue();
349-
if (context) {
350-
auto rawPtrType = builder.getASTContext().TheRawPointerType;
351-
auto rawPtrLoweredType = function.getLoweredLoadableType(rawPtrType);
352-
auto rawPtr = builder.createAddressToPointer(loc, context, rawPtrLoweredType);
353-
354-
auto ptrDecl = builder.getASTContext().getUnsafeRawPointerDecl();
355-
SILType ptrType = function.getLoweredType(ptrDecl->getDeclaredType());
356-
contextPtr = builder.createStruct(loc, ptrType, {rawPtr});
357-
}
288+
289+
assertHasNoContext();
290+
assert(getter->getArguments().size() == 2);
291+
assert(setter->getArguments().size() == 2);
358292

359293
// If this is a modify, we need to call the getter and
360294
// store the result in the writeback buffer.
361295
if (accessType == AccessType::Modify) {
362296
auto getterRef = builder.createFunctionRef(loc, getter);
363-
if (contextPtr)
364-
builder.createApply(loc, getterRef, subs, {addr, parent, contextPtr});
365-
else
366-
builder.createApply(loc, getterRef, subs, {addr, parent});
297+
builder.createApply(loc, getterRef, subs, {addr, parentValue});
367298
}
368299

369300
// The callback function will write into the writeback buffer.
370301
callback(addr);
371302

372303
// Pass the value from the writeback buffer to the setter.
373304
auto setterRef = builder.createFunctionRef(loc, setter);
374-
if (contextPtr)
375-
builder.createApply(loc, setterRef, subs, {addr, parent, contextPtr});
376-
else
377-
builder.createApply(loc, setterRef, subs, {addr, parent});
378-
379-
if (context) {
380-
builder.createDestroyAddr(loc, context);
381-
builder.createDeallocStack(loc, context);
382-
}
305+
builder.createApply(loc, setterRef, subs, {addr, parentValue});
383306

384307
// Deallocate the writeback buffer.
385308
builder.createDestroyAddr(loc, addr);
@@ -390,7 +313,6 @@ class SettablePropertyProjector : public GettablePropertyProjector {
390313
}
391314
};
392315

393-
394316
class OptionalWrapProjector : public ComponentProjector {
395317
public:
396318
OptionalWrapProjector(const KeyPathPatternComponent &component,
@@ -404,7 +326,7 @@ class OptionalWrapProjector : public ComponentProjector {
404326
KeyPathPatternComponent::Kind::OptionalWrap);
405327
assert(accessType == AccessType::Get && "optional wrap components are immutable");
406328

407-
parent->project(AccessType::Get, [&](SILValue parent) {
329+
parent->project(AccessType::Get, [&](SILValue parentValue) {
408330
auto &function = builder.getFunction();
409331
SILType optType = function.getLoweredType(component.getComponentType());
410332
SILType objType = optType.getOptionalObjectType().getAddressType();
@@ -418,7 +340,7 @@ class OptionalWrapProjector : public ComponentProjector {
418340
auto someDecl = builder.getASTContext().getOptionalSomeDecl();
419341
auto objAddr = builder.createInitEnumDataAddr(loc, optAddr,
420342
someDecl, objType);
421-
builder.createCopyAddr(loc, parent, objAddr, IsNotTake, IsInitialization);
343+
builder.createCopyAddr(loc, parentValue, objAddr, IsNotTake, IsInitialization);
422344

423345
// Initialize the Optional enum.
424346
builder.createInjectEnumAddr(loc, optAddr, someDecl);
@@ -578,9 +500,6 @@ class OptionalChainProjector : public ComponentProjector {
578500
SILValue optionalChainResult;
579501
};
580502

581-
582-
583-
584503
/// A projector to handle a complete key path.
585504
class CompleteKeyPathProjector : public KeyPathProjector {
586505
public:
@@ -717,6 +636,21 @@ KeyPathProjector::create(SILValue keyPath, SILValue root,
717636
auto *kpInst = dyn_cast<KeyPathInst>(keyPath);
718637
if (!kpInst || !kpInst->hasPattern())
719638
return nullptr;
639+
640+
// Check if the keypath only contains patterns which we support.
641+
auto components = kpInst->getPattern()->getComponents();
642+
for (const KeyPathPatternComponent &comp : components) {
643+
if (comp.getKind() == KeyPathPatternComponent::Kind::GettableProperty ||
644+
comp.getKind() == KeyPathPatternComponent::Kind::SettableProperty) {
645+
if (!comp.getExternalSubstitutions().empty() ||
646+
!comp.getSubscriptIndices().empty()) {
647+
// TODO: right now we can't optimize computed properties that require
648+
// additional context for subscript indices or generic environment
649+
// See https://github.com/apple/swift/pull/28799#issuecomment-570299845
650+
return nullptr;
651+
}
652+
}
653+
}
720654

721655
return std::make_unique<CompleteKeyPathProjector>(kpInst, root,
722656
loc, builder);

0 commit comments

Comments
 (0)