Skip to content

[sil-combine] Update sil combine for Ownership [part 1] #35246

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 11 commits into from
Jan 5, 2021
Merged

[sil-combine] Update sil combine for Ownership [part 1] #35246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f67c52e
[ownership] ref_to_* and *_to_ref produce values with OwnershipKind::…
gottesmm Dec 31, 2020
f6c7c0d
[ownership] bridge_object_to_word is a bitwise escape not an instanta…
gottesmm Dec 31, 2020
eb254f7
[sil-combine] Add ossa test translations that already pass given curr…
gottesmm Dec 31, 2020
e18243f
[sil-combine] Add a test for non-ossa to fix a test coverage hole tha…
gottesmm Dec 31, 2020
87e66ac
[sil-combine] Add translations of sil combine tests for OSSA disablin…
gottesmm Dec 31, 2020
c99874b
[sil-combine] Enable some checks that already pass in ossa_sil_combin…
gottesmm Dec 31, 2020
a5e7c3f
[sil-combine] Update LoadInst opts for OSSA and expand opts to also s…
gottesmm Dec 28, 2020
0a0f8cf
[sil-combine] Update switch_enum_addr -> switch_enum for loadable typ…
gottesmm Dec 28, 2020
2413f5f
[sil-combine] Update select_enum_addr canonicalization to use OSSA.
gottesmm Dec 28, 2020
5604831
[sil-combine] Enable switch_value to be optimized in OSSA.
gottesmm Dec 28, 2020
b206d00
Fix a comment in a test to be easier to read.
gottesmm Jan 4, 2021
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
15 changes: 11 additions & 4 deletions lib/SIL/IR/OperandOwnership.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,15 +186,11 @@ OPERAND_OWNERSHIP(InstantaneousUse, ValueMetatype)
OPERAND_OWNERSHIP(InstantaneousUse, IsEscapingClosure)
OPERAND_OWNERSHIP(InstantaneousUse, ClassMethod)
OPERAND_OWNERSHIP(InstantaneousUse, SuperMethod)
OPERAND_OWNERSHIP(InstantaneousUse, BridgeObjectToWord)
OPERAND_OWNERSHIP(InstantaneousUse, ClassifyBridgeObject)
OPERAND_OWNERSHIP(InstantaneousUse, SetDeallocating)
#define ALWAYS_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...) \
OPERAND_OWNERSHIP(InstantaneousUse, RefTo##Name) \
OPERAND_OWNERSHIP(InstantaneousUse, Name##ToRef) \
OPERAND_OWNERSHIP(InstantaneousUse, StrongCopy##Name##Value)
#define UNCHECKED_REF_STORAGE(Name, ...) \
OPERAND_OWNERSHIP(InstantaneousUse, RefTo##Name) \
OPERAND_OWNERSHIP(InstantaneousUse, StrongCopy##Name##Value)
#include "swift/AST/ReferenceStorage.def"

Expand All @@ -208,6 +204,16 @@ OPERAND_OWNERSHIP(UnownedInstantaneousUse, UnmanagedRetainValue)
OPERAND_OWNERSHIP(UnownedInstantaneousUse, UnmanagedReleaseValue)
OPERAND_OWNERSHIP(UnownedInstantaneousUse, UnmanagedAutoreleaseValue)

// These act as a form of conversion that does not imply ownership. Thus from an
// operand perspective we treat them as a pointer escape and from a value
// perspective, they return a value with OwnershipKind::Unowned.
#define ALWAYS_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...) \
OPERAND_OWNERSHIP(PointerEscape, RefTo##Name) \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atrick this is what I was talking about.

OPERAND_OWNERSHIP(PointerEscape, Name##ToRef)
#define UNCHECKED_REF_STORAGE(Name, ...) \
OPERAND_OWNERSHIP(PointerEscape, RefTo##Name)
#include "swift/AST/ReferenceStorage.def"

// Instructions that currently violate structural ownership requirements,
// and therefore completely defeat canonicalization and optimization of any
// OSSA value that they use.
Expand All @@ -222,6 +228,7 @@ OPERAND_OWNERSHIP(BitwiseEscape, UncheckedBitwiseCast)
OPERAND_OWNERSHIP(BitwiseEscape, ValueToBridgeObject)
OPERAND_OWNERSHIP(BitwiseEscape, RefToRawPointer)
OPERAND_OWNERSHIP(BitwiseEscape, UncheckedTrivialBitCast)
OPERAND_OWNERSHIP(BitwiseEscape, BridgeObjectToWord)

// Instructions that end the lifetime of an owned value.
OPERAND_OWNERSHIP(DestroyingConsume, AutoreleaseValue)
Expand Down
6 changes: 5 additions & 1 deletion lib/SILOptimizer/SILCombiner/SILCombiner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,12 @@ class SILCombiner :
SILInstruction *visitStrongRetainInst(StrongRetainInst *SRI);
SILInstruction *visitRefToRawPointerInst(RefToRawPointerInst *RRPI);
SILInstruction *visitUpcastInst(UpcastInst *UCI);
SILInstruction *optimizeLoadFromStringLiteral(LoadInst *LI);

// NOTE: The load optimized in this method is a load [trivial].
SILInstruction *optimizeLoadFromStringLiteral(LoadInst *li);

SILInstruction *visitLoadInst(LoadInst *LI);
SILInstruction *visitLoadBorrowInst(LoadBorrowInst *LI);
SILInstruction *visitIndexAddrInst(IndexAddrInst *IA);
bool optimizeStackAllocatedEnum(AllocStackInst *AS);
SILInstruction *visitAllocStackInst(AllocStackInst *AS);
Expand Down
225 changes: 152 additions & 73 deletions lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,6 @@ static EnumElementDecl *getInjectEnumCaseTo(SILValue Addr) {
}

SILInstruction *SILCombiner::visitSwitchEnumAddrInst(SwitchEnumAddrInst *SEAI) {
if (SEAI->getFunction()->hasOwnership())
return nullptr;

// Convert switch_enum_addr -> br
// if the only thing which writes to the address is an inject_enum_addr.
SILValue Addr = SEAI->getOperand();
Expand All @@ -172,41 +169,66 @@ SILInstruction *SILCombiner::visitSwitchEnumAddrInst(SwitchEnumAddrInst *SEAI) {
// ->
// %value = load %ptr
// switch_enum %value
SmallVector<std::pair<EnumElementDecl*, SILBasicBlock*>, 8> Cases;
for (int i = 0, e = SEAI->getNumCases(); i < e; ++i)
//
// If we are using ownership, we perform a load_borrow right before the new
// switch_enum and end the borrow scope right afterwards.
Builder.setCurrentDebugScope(SEAI->getDebugScope());
SmallVector<std::pair<EnumElementDecl *, SILBasicBlock *>, 8> Cases;
for (int i : range(SEAI->getNumCases())) {
Cases.push_back(SEAI->getCase(i));
}

Builder.setCurrentDebugScope(SEAI->getDebugScope());
SILBasicBlock *Default = SEAI->hasDefault() ? SEAI->getDefaultBB() : nullptr;
LoadInst *EnumVal = Builder.createLoad(SEAI->getLoc(), Addr,
LoadOwnershipQualifier::Unqualified);
Builder.createSwitchEnum(SEAI->getLoc(), EnumVal, Default, Cases);
SILValue EnumVal = Builder.emitLoadBorrowOperation(SEAI->getLoc(), Addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a while to figure out how this is safe. It would help to comment that this only creates a trivial load-borrow scope to cover the switch_enum terminator and its block argument results, so nothing can write to memory within this scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked, I do have a comment a line or two above:

  // Promote switch_enum_addr to switch_enum if the enum is loadable.                                                                    
  //   switch_enum_addr %ptr : $*Optional<SomeClass>, case ...                                                                           
  //     ->                                                                                                                              
  //   %value = load %ptr                                                                                                                
  //   switch_enum %value                                                                                                                
  //                                                                                                                                     
  // If we are using ownership, we perform a load_borrow right before the new                                                            
  // switch_enum and end the borrow scope right afterwards.

auto *sei = Builder.createSwitchEnum(SEAI->getLoc(), EnumVal, Default, Cases);

if (Builder.hasOwnership()) {
for (int i : range(sei->getNumCases())) {
auto c = sei->getCase(i);
if (c.first->hasAssociatedValues()) {
auto eltType = Addr->getType().getEnumElementType(
c.first, Builder.getModule(), Builder.getTypeExpansionContext());
eltType = eltType.getObjectType();
if (eltType.isTrivial(Builder.getFunction())) {
c.second->createPhiArgument(eltType, OwnershipKind::None);
} else {
c.second->createPhiArgument(eltType, OwnershipKind::Guaranteed);
}
}
Builder.setInsertionPoint(c.second->front().getIterator());
Builder.emitEndBorrowOperation(SEAI->getLoc(), EnumVal);
}

if (auto defaultBlock = sei->getDefaultBBOrNull()) {
defaultBlock.get()->createPhiArgument(EnumVal->getType(),
OwnershipKind::Guaranteed);
Builder.setInsertionPoint(defaultBlock.get()->front().getIterator());
Builder.emitEndBorrowOperation(SEAI->getLoc(), EnumVal);
}
}

return eraseInstFromFunction(*SEAI);
}

SILInstruction *SILCombiner::visitSelectEnumAddrInst(SelectEnumAddrInst *SEAI) {
if (SEAI->getFunction()->hasOwnership())
return nullptr;

SILInstruction *SILCombiner::visitSelectEnumAddrInst(SelectEnumAddrInst *seai) {
// Canonicalize a select_enum_addr: if the default refers to exactly one case,
// then replace the default with that case.
Builder.setCurrentDebugScope(SEAI->getDebugScope());
if (SEAI->hasDefault()) {
NullablePtr<EnumElementDecl> elementDecl = SEAI->getUniqueCaseForDefault();
Builder.setCurrentDebugScope(seai->getDebugScope());
if (seai->hasDefault()) {
NullablePtr<EnumElementDecl> elementDecl = seai->getUniqueCaseForDefault();
if (elementDecl.isNonNull()) {
// Construct a new instruction by copying all the case entries.
SmallVector<std::pair<EnumElementDecl *, SILValue>, 4> CaseValues;
for (int idx = 0, numIdcs = SEAI->getNumCases(); idx < numIdcs; ++idx) {
CaseValues.push_back(SEAI->getCase(idx));
SmallVector<std::pair<EnumElementDecl *, SILValue>, 4> caseValues;
for (int idx = 0, numIdcs = seai->getNumCases(); idx < numIdcs; ++idx) {
caseValues.push_back(seai->getCase(idx));
}
// Add the default-entry of the original instruction as case-entry.
CaseValues.push_back(
std::make_pair(elementDecl.get(), SEAI->getDefaultResult()));
caseValues.push_back(
std::make_pair(elementDecl.get(), seai->getDefaultResult()));

return Builder.createSelectEnumAddr(SEAI->getLoc(),
SEAI->getEnumOperand(),
SEAI->getType(), SILValue(),
CaseValues);
return Builder.createSelectEnumAddr(
seai->getLoc(), seai->getEnumOperand(), seai->getType(), SILValue(),
caseValues);
}
}

Expand All @@ -215,63 +237,65 @@ SILInstruction *SILCombiner::visitSelectEnumAddrInst(SelectEnumAddrInst *SEAI) {
// ->
// %value = load %ptr
// = select_enum %value
SILType Ty = SEAI->getEnumOperand()->getType();
if (!Ty.isLoadable(*SEAI->getFunction()))
SILType ty = seai->getEnumOperand()->getType();
if (!ty.isLoadable(*seai->getFunction()))
return nullptr;

SmallVector<std::pair<EnumElementDecl*, SILValue>, 8> Cases;
for (int i = 0, e = SEAI->getNumCases(); i < e; ++i)
Cases.push_back(SEAI->getCase(i));

SILValue Default = SEAI->hasDefault() ? SEAI->getDefaultResult() : SILValue();
LoadInst *EnumVal = Builder.createLoad(SEAI->getLoc(), SEAI->getEnumOperand(),
LoadOwnershipQualifier::Unqualified);
auto *I = Builder.createSelectEnum(SEAI->getLoc(), EnumVal, SEAI->getType(),
Default, Cases);
return I;
SmallVector<std::pair<EnumElementDecl *, SILValue>, 8> cases;
for (int i = 0, e = seai->getNumCases(); i < e; ++i)
cases.push_back(seai->getCase(i));

SILValue defaultCase =
seai->hasDefault() ? seai->getDefaultResult() : SILValue();
auto enumVal =
Builder.emitLoadBorrowOperation(seai->getLoc(), seai->getEnumOperand());
auto *result = Builder.createSelectEnum(seai->getLoc(), enumVal,
seai->getType(), defaultCase, cases);
Builder.emitEndBorrowOperation(seai->getLoc(), enumVal);
replaceInstUsesWith(*seai, result);
return eraseInstFromFunction(*seai);
}

SILInstruction *SILCombiner::visitSwitchValueInst(SwitchValueInst *SVI) {
if (SVI->getFunction()->hasOwnership())
SILInstruction *SILCombiner::visitSwitchValueInst(SwitchValueInst *svi) {
SILValue cond = svi->getOperand();
BuiltinIntegerType *condTy = cond->getType().getAs<BuiltinIntegerType>();
if (!condTy || !condTy->isFixedWidth(1))
return nullptr;

SILValue Cond = SVI->getOperand();
BuiltinIntegerType *CondTy = Cond->getType().getAs<BuiltinIntegerType>();
if (!CondTy || !CondTy->isFixedWidth(1))
return nullptr;

SILBasicBlock *FalseBB = nullptr;
SILBasicBlock *TrueBB = nullptr;
for (unsigned Idx = 0, Num = SVI->getNumCases(); Idx < Num; ++Idx) {
auto Case = SVI->getCase(Idx);
auto *CaseVal = dyn_cast<IntegerLiteralInst>(Case.first);
if (!CaseVal)
SILBasicBlock *falseBB = nullptr;
SILBasicBlock *trueBB = nullptr;
for (unsigned idx : range(svi->getNumCases())) {
auto switchCase = svi->getCase(idx);
auto *caseVal = dyn_cast<IntegerLiteralInst>(switchCase.first);
if (!caseVal)
return nullptr;
SILBasicBlock *DestBB = Case.second;
assert(DestBB->args_empty() &&
SILBasicBlock *destBB = switchCase.second;
assert(destBB->args_empty() &&
"switch_value case destination cannot take arguments");
if (CaseVal->getValue() == 0) {
assert(!FalseBB && "double case value 0 in switch_value");
FalseBB = DestBB;
if (caseVal->getValue() == 0) {
assert(!falseBB && "double case value 0 in switch_value");
falseBB = destBB;
} else {
assert(!TrueBB && "double case value 1 in switch_value");
TrueBB = DestBB;
assert(!trueBB && "double case value 1 in switch_value");
trueBB = destBB;
}
}
if (SVI->hasDefault()) {
assert(SVI->getDefaultBB()->args_empty() &&

if (svi->hasDefault()) {
assert(svi->getDefaultBB()->args_empty() &&
"switch_value default destination cannot take arguments");
if (!FalseBB) {
FalseBB = SVI->getDefaultBB();
} else if (!TrueBB) {
TrueBB = SVI->getDefaultBB();
if (!falseBB) {
falseBB = svi->getDefaultBB();
} else if (!trueBB) {
trueBB = svi->getDefaultBB();
}
}
if (!FalseBB || !TrueBB)

if (!falseBB || !trueBB)
return nullptr;

Builder.setCurrentDebugScope(SVI->getDebugScope());
return Builder.createCondBranch(SVI->getLoc(), Cond, TrueBB, FalseBB);
Builder.setCurrentDebugScope(svi->getDebugScope());
return Builder.createCondBranch(svi->getLoc(), cond, trueBB, falseBB);
}

namespace {
Expand Down Expand Up @@ -759,12 +783,13 @@ SILInstruction *SILCombiner::optimizeLoadFromStringLiteral(LoadInst *LI) {

/// Returns true if \p LI loads a zero integer from the empty Array, Dictionary
/// or Set singleton.
static bool isZeroLoadFromEmptyCollection(LoadInst *LI) {
static bool isZeroLoadFromEmptyCollection(SingleValueInstruction *LI) {
assert(isa<LoadInst>(LI) || isa<LoadBorrowInst>(LI));
auto intTy = LI->getType().getAs<BuiltinIntegerType>();
if (!intTy)
return false;
SILValue addr = LI->getOperand();

SILValue addr = LI->getOperand(0);

// Find the root object of the load-address.
for (;;) {
Expand Down Expand Up @@ -805,6 +830,8 @@ static bool isZeroLoadFromEmptyCollection(LoadInst *LI) {
case ValueKind::UpcastInst:
case ValueKind::RawPointerToRefInst:
case ValueKind::AddressToPointerInst:
case ValueKind::BeginBorrowInst:
case ValueKind::CopyValueInst:
Copy link
Contributor

Choose a reason for hiding this comment

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

fyi, this is just an example of one of the 100+ places we should use AccessedStorageVisitor so we're not maintaining them all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Maybe I can fix that in a subsequent commit.

case ValueKind::EndCOWMutationInst:
addr = cast<SingleValueInstruction>(addr)->getOperand(0);
break;
Expand Down Expand Up @@ -839,15 +866,56 @@ static SingleValueInstruction *getValueFromStaticLet(SILValue v) {
return nullptr;
}

SILInstruction *SILCombiner::visitLoadInst(LoadInst *LI) {
if (LI->getFunction()->hasOwnership())
SILInstruction *SILCombiner::visitLoadBorrowInst(LoadBorrowInst *lbi) {
// (load (upcast-ptr %x)) -> (upcast-ref (load %x))
Builder.setCurrentDebugScope(lbi->getDebugScope());
if (auto *ui = dyn_cast<UpcastInst>(lbi->getOperand())) {
// We want to RAUW the current load_borrow with the upcast. To do that
// safely, we need to insert new end_borrow on the new load_borrow, erase
// the end_borrow and then RAUW.
SmallVector<EndBorrowInst *, 32> endBorrowInst;
for (auto *ebi : lbi->getEndBorrows())
endBorrowInst.push_back(ebi);
auto newLBI = Builder.createLoadBorrow(lbi->getLoc(), ui->getOperand());
for (auto *ebi : endBorrowInst) {
SILBuilderWithScope builder(ebi, Builder);
builder.emitEndBorrowOperation(ebi->getLoc(), newLBI);
eraseInstFromFunction(*ebi);
}
auto *uci = Builder.createUpcast(lbi->getLoc(), newLBI, lbi->getType());
replaceInstUsesWith(*lbi, uci);
return eraseInstFromFunction(*lbi);
}

// Constant-propagate the 0 value when loading "count" or "capacity" from the
// empty Array, Set or Dictionary storage.
// On high-level SIL this optimization is also done by the
// ArrayCountPropagation pass, but only for Array. And even for Array it's
// sometimes needed to propagate the empty-array count when high-level
// semantics function are already inlined.
// Note that for non-empty arrays/sets/dictionaries, the count can be
// propagated by redundant load elimination.
if (isZeroLoadFromEmptyCollection(lbi))
return Builder.createIntegerLiteral(lbi->getLoc(), lbi->getType(), 0);

// If we have a load_borrow that only has non_debug end_borrow uses, delete
// it.
if (llvm::all_of(getNonDebugUses(lbi), [](Operand *use) {
return isa<EndBorrowInst>(use->getUser());
})) {
eraseInstIncludingUsers(lbi);
return nullptr;
}

return nullptr;
}

SILInstruction *SILCombiner::visitLoadInst(LoadInst *LI) {
// (load (upcast-ptr %x)) -> (upcast-ref (load %x))
Builder.setCurrentDebugScope(LI->getDebugScope());
if (auto *UI = dyn_cast<UpcastInst>(LI->getOperand())) {
auto NewLI = Builder.createLoad(LI->getLoc(), UI->getOperand(),
LoadOwnershipQualifier::Unqualified);
auto NewLI = Builder.emitLoadValueOperation(LI->getLoc(), UI->getOperand(),
LI->getOwnershipQualifier());
return Builder.createUpcast(LI->getLoc(), NewLI, LI->getType());
}

Expand All @@ -874,6 +942,17 @@ SILInstruction *SILCombiner::visitLoadInst(LoadInst *LI) {
return cloner.clone(initVal);
}

// If we have a load [copy] whose only non-debug users are destroy_value, just
// eliminate it.
if (LI->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
if (llvm::all_of(getNonDebugUses(LI), [](Operand *use) {
return isa<DestroyValueInst>(use->getUser());
})) {
eraseInstIncludingUsers(LI);
return nullptr;
}
}

return nullptr;
}

Expand Down
14 changes: 14 additions & 0 deletions test/SIL/ownership-verifier/use_verifier.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1417,3 +1417,17 @@ bb3(%fUnknown : @owned $@callee_owned () -> ()):
%9999 = tuple()
return %9999 : $()
}

sil [ossa] @unowned_to_ref_is_unowned_instant_use : $@convention(thin) (@guaranteed Builtin.NativeObject) -> Builtin.NativeObject {
bb0(%0 : @guaranteed $Builtin.NativeObject):
%1 = ref_to_unowned %0 : $Builtin.NativeObject to $@sil_unowned Builtin.NativeObject
%2 = unowned_to_ref %1 : $@sil_unowned Builtin.NativeObject to $Builtin.NativeObject
return %2 : $Builtin.NativeObject
}

sil [ossa] @unmanaged_to_ref_is_unowned_instant_use : $@convention(thin) (@guaranteed Builtin.NativeObject) -> Builtin.NativeObject {
bb0(%0 : @guaranteed $Builtin.NativeObject):
%1 = ref_to_unmanaged %0 : $Builtin.NativeObject to $@sil_unmanaged Builtin.NativeObject
%2 = unmanaged_to_ref %1 : $@sil_unmanaged Builtin.NativeObject to $Builtin.NativeObject
return %2 : $Builtin.NativeObject
}
Loading