Skip to content

A couple of SIL fixes #34267

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 3 commits into from
Oct 11, 2020
Merged

A couple of SIL fixes #34267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7bc5c5e
SIL: Fix ownership verifier to handle some missing interior pointer p…
slavapestov Oct 9, 2020
5096345
DI: Handle instructions that initialize multiple tuple elements
slavapestov Oct 10, 2020
53b91da
DI: Remove scalarization 'pass'
slavapestov Oct 10, 2020
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
8 changes: 5 additions & 3 deletions lib/SIL/Utils/OwnershipUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,16 +543,18 @@ bool InteriorPointerOperand::getImplicitUses(
isa<BeginUnpairedAccessInst>(user) ||
isa<EndUnpairedAccessInst>(user) || isa<WitnessMethodInst>(user) ||
isa<SwitchEnumAddrInst>(user) || isa<CheckedCastAddrBranchInst>(user) ||
isa<SelectEnumAddrInst>(user)) {
isa<SelectEnumAddrInst>(user) || isa<InjectEnumAddrInst>(user)) {
continue;
}

// Then handle users that we need to look at transitive uses of.
if (Projection::isAddressProjection(user) ||
isa<ProjectBlockStorageInst>(user) ||
isa<OpenExistentialAddrInst>(user) ||
isa<InitExistentialAddrInst>(user) || isa<BeginAccessInst>(user) ||
isa<TailAddrInst>(user) || isa<IndexAddrInst>(user)) {
isa<InitExistentialAddrInst>(user) ||
isa<InitEnumDataAddrInst>(user) || isa<BeginAccessInst>(user) ||
isa<TailAddrInst>(user) || isa<IndexAddrInst>(user) ||
isa<UnconditionalCheckedCastAddrInst>(user)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@slavapestov This is incorrect. UnconditionalCheckedCastAddrInst does not have any results. Instead, you need to select its uses directly. Can you add an error test for unconditional_checked_cast_addr as well just to show via a test that we are correct here?

for (SILValue r : user->getResults()) {
llvm::copy(r->getUses(), std::back_inserter(worklist));
}
Expand Down
2 changes: 1 addition & 1 deletion lib/SIL/Verifier/SILOwnershipVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ bool SILValueOwnershipChecker::gatherUsers(
llvm::errs() << "Could not recognize address user of interior "
"pointer operand!\n"
<< "Interior Pointer Operand: "
<< interiorPointerOperand->operand->getUser()
<< *interiorPointerOperand->operand->getUser()
<< "Address User: " << *op->getUser();
});
};
Expand Down
148 changes: 1 addition & 147 deletions lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,48 +466,6 @@ void DIElementUseInfo::trackStoreToSelf(SILInstruction *I) {
StoresToSelf.push_back(I);
}

//===----------------------------------------------------------------------===//
// Scalarization Logic
//===----------------------------------------------------------------------===//

/// Given a pointer to a tuple type, compute the addresses of each element and
/// add them to the ElementAddrs vector.
static void
getScalarizedElementAddresses(SILValue Pointer, SILBuilder &B, SILLocation Loc,
SmallVectorImpl<SILValue> &ElementAddrs) {
TupleType *TT = Pointer->getType().castTo<TupleType>();
for (auto Index : indices(TT->getElements())) {
ElementAddrs.push_back(B.createTupleElementAddr(Loc, Pointer, Index));
}
}

/// Given an RValue of aggregate type, compute the values of the elements by
/// emitting a destructure.
static void getScalarizedElements(SILValue V,
SmallVectorImpl<SILValue> &ElementVals,
SILLocation Loc, SILBuilder &B) {
auto *DTI = B.createDestructureTuple(Loc, V);
llvm::copy(DTI->getResults(), std::back_inserter(ElementVals));
}

/// Scalarize a load down to its subelements. If NewLoads is specified, this
/// can return the newly generated sub-element loads.
static SILValue scalarizeLoad(LoadInst *LI,
SmallVectorImpl<SILValue> &ElementAddrs) {
SILBuilderWithScope B(LI);
SmallVector<SILValue, 4> ElementTmps;

for (unsigned i = 0, e = ElementAddrs.size(); i != e; ++i) {
auto *SubLI = B.createTrivialLoadOr(LI->getLoc(), ElementAddrs[i],
LI->getOwnershipQualifier());
ElementTmps.push_back(SubLI);
}

if (LI->getType().is<TupleType>())
return B.createTuple(LI->getLoc(), LI->getType(), ElementTmps);
return B.createStruct(LI->getLoc(), LI->getType(), ElementTmps);
}

//===----------------------------------------------------------------------===//
// ElementUseCollector Implementation
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -671,11 +629,6 @@ void ElementUseCollector::collectUses(SILValue Pointer, unsigned BaseEltNo) {
"Walked through the pointer to the value?");
SILType PointeeType = Pointer->getType().getObjectType();

// This keeps track of instructions in the use list that touch multiple tuple
// elements and should be scalarized. This is done as a second phase to
// avoid invalidating the use iterator.
SmallVector<SILInstruction *, 4> UsesToScalarize;

for (auto *Op : Pointer->getUses()) {
auto *User = Op->getUser();

Expand Down Expand Up @@ -705,10 +658,7 @@ void ElementUseCollector::collectUses(SILValue Pointer, unsigned BaseEltNo) {

// Loads are a use of the value.
if (isa<LoadInst>(User)) {
if (PointeeType.is<TupleType>())
UsesToScalarize.push_back(User);
else
addElementUses(BaseEltNo, PointeeType, User, DIUseKind::Load);
addElementUses(BaseEltNo, PointeeType, User, DIUseKind::Load);
continue;
}

Expand All @@ -730,13 +680,6 @@ void ElementUseCollector::collectUses(SILValue Pointer, unsigned BaseEltNo) {
if ((isa<StoreInst>(User) || isa<AssignInst>(User) ||
isa<AssignByWrapperInst>(User)) &&
Op->getOperandNumber() == 1) {
if (PointeeType.is<TupleType>()) {
assert(!isa<AssignByWrapperInst>(User) &&
"cannot assign a typle with assign_by_wrapper");
UsesToScalarize.push_back(User);
continue;
}

// Coming out of SILGen, we assume that raw stores are initializations,
// unless they have trivial type (which we classify as InitOrAssign).
DIUseKind Kind;
Expand Down Expand Up @@ -769,13 +712,6 @@ void ElementUseCollector::collectUses(SILValue Pointer, unsigned BaseEltNo) {
#include "swift/AST/ReferenceStorage.def"

if (auto *CAI = dyn_cast<CopyAddrInst>(User)) {
// If this is a copy of a tuple, we should scalarize it so that we don't
// have an access that crosses elements.
if (PointeeType.is<TupleType>()) {
UsesToScalarize.push_back(CAI);
continue;
}

// If this is the source of the copy_addr, then this is a load. If it is
// the destination, then this is an unknown assignment. Note that we'll
// revisit this instruction and add it to Uses twice if it is both a load
Expand Down Expand Up @@ -988,88 +924,6 @@ void ElementUseCollector::collectUses(SILValue Pointer, unsigned BaseEltNo) {
// Otherwise, the use is something complicated, it escapes.
addElementUses(BaseEltNo, PointeeType, User, DIUseKind::Escape);
}

// Now that we've walked all of the immediate uses, scalarize any operations
// working on tuples if we need to for canonicalization or analysis reasons.
if (!UsesToScalarize.empty()) {
SILInstruction *PointerInst = Pointer->getDefiningInstruction();
SmallVector<SILValue, 4> ElementAddrs;
SILBuilderWithScope AddrBuilder(++SILBasicBlock::iterator(PointerInst),
PointerInst);
getScalarizedElementAddresses(Pointer, AddrBuilder, PointerInst->getLoc(),
ElementAddrs);

SmallVector<SILValue, 4> ElementTmps;
for (auto *User : UsesToScalarize) {
ElementTmps.clear();

LLVM_DEBUG(llvm::errs() << " *** Scalarizing: " << *User << "\n");

// Scalarize LoadInst
if (auto *LI = dyn_cast<LoadInst>(User)) {
SILValue Result = scalarizeLoad(LI, ElementAddrs);
LI->replaceAllUsesWith(Result);
LI->eraseFromParent();
continue;
}

// Scalarize AssignInst
if (auto *AI = dyn_cast<AssignInst>(User)) {
SILBuilderWithScope B(User, AI);
getScalarizedElements(AI->getOperand(0), ElementTmps, AI->getLoc(), B);

for (unsigned i = 0, e = ElementAddrs.size(); i != e; ++i)
B.createAssign(AI->getLoc(), ElementTmps[i], ElementAddrs[i],
AssignOwnershipQualifier::Unknown);
AI->eraseFromParent();
continue;
}

// Scalarize StoreInst
if (auto *SI = dyn_cast<StoreInst>(User)) {
SILBuilderWithScope B(User, SI);
getScalarizedElements(SI->getOperand(0), ElementTmps, SI->getLoc(), B);

for (unsigned i = 0, e = ElementAddrs.size(); i != e; ++i)
B.createTrivialStoreOr(SI->getLoc(), ElementTmps[i], ElementAddrs[i],
SI->getOwnershipQualifier());
SI->eraseFromParent();
continue;
}

// Scalarize CopyAddrInst.
auto *CAI = cast<CopyAddrInst>(User);
SILBuilderWithScope B(User, CAI);

// Determine if this is a copy *from* or *to* "Pointer".
if (CAI->getSrc() == Pointer) {
// Copy from pointer.
getScalarizedElementAddresses(CAI->getDest(), B, CAI->getLoc(),
ElementTmps);
for (unsigned i = 0, e = ElementAddrs.size(); i != e; ++i)
B.createCopyAddr(CAI->getLoc(), ElementAddrs[i], ElementTmps[i],
CAI->isTakeOfSrc(), CAI->isInitializationOfDest());

} else {
getScalarizedElementAddresses(CAI->getSrc(), B, CAI->getLoc(),
ElementTmps);
for (unsigned i = 0, e = ElementAddrs.size(); i != e; ++i)
B.createCopyAddr(CAI->getLoc(), ElementTmps[i], ElementAddrs[i],
CAI->isTakeOfSrc(), CAI->isInitializationOfDest());
}
CAI->eraseFromParent();
}

// Now that we've scalarized some stuff, recurse down into the newly created
// element address computations to recursively process it. This can cause
// further scalarization.
for (SILValue EltPtr : ElementAddrs) {
if (auto *TEAI = dyn_cast<TupleElementAddrInst>(EltPtr)) {
collectTupleElementUses(TEAI, BaseEltNo);
continue;
}
}
}
}

/// collectClassSelfUses - Collect all the uses of a 'self' pointer in a class
Expand Down
58 changes: 31 additions & 27 deletions lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ namespace {

/// This is a map of uses that are not loads (i.e., they are Stores,
/// InOutUses, and Escapes), to their entry in Uses.
llvm::SmallDenseMap<SILInstruction*, unsigned, 16> NonLoadUses;
llvm::SmallDenseMap<SILInstruction*, SmallVector<unsigned, 1>, 16> NonLoadUses;

/// This is true when there is an ambiguous store, which may be an init or
/// assign, depending on the CFG path.
Expand Down Expand Up @@ -472,7 +472,7 @@ namespace {

void handleSelfInitUse(unsigned UseID);

void updateInstructionForInitState(DIMemoryUse &Use);
void updateInstructionForInitState(unsigned UseID);


void processUninitializedRelease(SILInstruction *Release,
Expand Down Expand Up @@ -544,7 +544,7 @@ LifetimeChecker::LifetimeChecker(const DIMemoryObjectInfo &TheMemory,
break;
}

NonLoadUses[Use.Inst] = ui;
NonLoadUses[Use.Inst].push_back(ui);

auto &BBInfo = getBlockInfo(Use.Inst->getParent());
BBInfo.HasNonLoadUse = true;
Expand All @@ -562,9 +562,8 @@ LifetimeChecker::LifetimeChecker(const DIMemoryObjectInfo &TheMemory,
getBlockInfo(bb).markStoreToSelf();
}

// If isn't really a use, but we account for the mark_uninitialized or
// It isn't really a use, but we account for the mark_uninitialized or
// project_box as a use so we see it in our dataflow walks.
NonLoadUses[TheMemory.getUninitializedValue()] = ~0U;
auto &MemBBInfo = getBlockInfo(TheMemory.getParentBlock());
MemBBInfo.HasNonLoadUse = true;

Expand Down Expand Up @@ -826,7 +825,7 @@ void LifetimeChecker::doIt() {
// postpone lowering of assignment instructions to avoid deleting
// instructions that still appear in the Uses list.
for (unsigned UseID : NeedsUpdateForInitState)
updateInstructionForInitState(Uses[UseID]);
updateInstructionForInitState(UseID);
}

void LifetimeChecker::handleLoadUse(const DIMemoryUse &Use) {
Expand Down Expand Up @@ -1911,7 +1910,8 @@ void LifetimeChecker::handleSelfInitUse(unsigned UseID) {
/// from being InitOrAssign to some concrete state, update it for that state.
/// This includes rewriting them from assign instructions into their composite
/// operations.
void LifetimeChecker::updateInstructionForInitState(DIMemoryUse &Use) {
void LifetimeChecker::updateInstructionForInitState(unsigned UseID) {
DIMemoryUse &Use = Uses[UseID];
SILInstruction *Inst = Use.Inst;

IsInitialization_t InitKind;
Expand Down Expand Up @@ -1945,10 +1945,11 @@ void LifetimeChecker::updateInstructionForInitState(DIMemoryUse &Use) {
// If this is an assign, rewrite it based on whether it is an initialization
// or not.
if (auto *AI = dyn_cast<AssignInst>(Inst)) {

// Remove this instruction from our data structures, since we will be
// removing it.
Use.Inst = nullptr;
NonLoadUses.erase(Inst);
llvm::erase_if(NonLoadUses[Inst], [&](unsigned id) { return id == UseID; });

if (TheMemory.isClassInitSelf() &&
Use.Kind == DIUseKind::SelfInit) {
Expand All @@ -1966,7 +1967,7 @@ void LifetimeChecker::updateInstructionForInitState(DIMemoryUse &Use) {
// Remove this instruction from our data structures, since we will be
// removing it.
Use.Inst = nullptr;
NonLoadUses.erase(Inst);
llvm::erase_if(NonLoadUses[Inst], [&](unsigned id) { return id == UseID; });

switch (Use.Kind) {
case DIUseKind::Initialization:
Expand Down Expand Up @@ -2819,17 +2820,17 @@ LifetimeChecker::getLivenessAtNonTupleInst(swift::SILInstruction *Inst,
--BBI;
SILInstruction *TheInst = &*BBI;

// If this instruction is unrelated to the memory, ignore it.
if (!NonLoadUses.count(TheInst))
continue;
if (TheInst == TheMemory.getUninitializedValue()) {
Result.set(0, DIKind::No);
return Result;
}

// If we found the allocation itself, then we are loading something that
// is not defined at all yet. Otherwise, we've found a definition, or
// something else that will require that the memory is initialized at
// this point.
Result.set(0, TheInst == TheMemory.getUninitializedValue() ? DIKind::No
: DIKind::Yes);
return Result;
if (NonLoadUses.count(TheInst)) {
// We've found a definition, or something else that will require that
// the memory is initialized at this point.
Result.set(0, DIKind::Yes);
return Result;
}
}
}

Expand Down Expand Up @@ -2882,11 +2883,6 @@ AvailabilitySet LifetimeChecker::getLivenessAtInst(SILInstruction *Inst,
--BBI;
SILInstruction *TheInst = &*BBI;

// If this instruction is unrelated to the memory, ignore it.
auto It = NonLoadUses.find(TheInst);
if (It == NonLoadUses.end())
continue;

// If we found the allocation itself, then we are loading something that
// is not defined at all yet. Scan no further.
if (TheInst == TheMemory.getUninitializedValue()) {
Expand All @@ -2896,11 +2892,19 @@ AvailabilitySet LifetimeChecker::getLivenessAtInst(SILInstruction *Inst,
return Result;
}

// If this instruction is unrelated to the memory, ignore it.
auto It = NonLoadUses.find(TheInst);
if (It == NonLoadUses.end())
continue;

// Check to see which tuple elements this instruction defines. Clear them
// from the set we're scanning from.
auto &TheInstUse = Uses[It->second];
NeededElements.reset(TheInstUse.FirstElement,
TheInstUse.FirstElement+TheInstUse.NumElements);
for (unsigned TheUse : It->second) {
auto &TheInstUse = Uses[TheUse];
NeededElements.reset(TheInstUse.FirstElement,
TheInstUse.FirstElement+TheInstUse.NumElements);
}

// If that satisfied all of the elements we're looking for, then we're
// done. Otherwise, keep going.
if (NeededElements.none()) {
Expand Down
Loading