Skip to content

[move-function] Implement move checking for address only lets #40423

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.

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 5 commits into from
Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion include/swift/AST/DiagnosticsSIL.def
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,7 @@ NOTE(sil_movekillscopyablevalue_use_here, none,
NOTE(sil_movekillscopyablevalue_value_consumed_in_loop, none,
"cyclic move here. move will occur multiple times in the loop", ())
ERROR(sil_movekillscopyablevalue_move_applied_to_unsupported_move, none,
"_move applied to value that the compiler does not supporting checking.",
"_move applied to value that the compiler does not support checking",
())

#define UNDEFINE_DIAGNOSTIC_MACROS
Expand Down
4 changes: 4 additions & 0 deletions include/swift/SIL/BasicBlockDatastructures.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ class BasicBlockSetVector {
iterator begin() const { return vector.begin(); }
iterator end() const { return vector.end(); }

llvm::iterator_range<iterator> getRange() const {
return llvm::make_range(begin(), end());
}

bool empty() const { return vector.empty(); }

bool contains(SILBasicBlock *block) const { return set.contains(block); }
Expand Down
7 changes: 7 additions & 0 deletions include/swift/SIL/SILBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -1271,6 +1271,13 @@ class SILBuilder {
MoveValueInst(getSILDebugLocation(loc), operand));
}

MarkUnresolvedMoveAddrInst *createMarkUnresolvedMoveAddr(SILLocation loc,
SILValue srcAddr,
SILValue takeAddr) {
return insert(new (getModule()) MarkUnresolvedMoveAddrInst(
getSILDebugLocation(loc), srcAddr, takeAddr));
}

UnconditionalCheckedCastInst *
createUnconditionalCheckedCast(SILLocation Loc, SILValue op,
SILType destLoweredTy,
Expand Down
10 changes: 10 additions & 0 deletions include/swift/SIL/SILCloner.h
Original file line number Diff line number Diff line change
Expand Up @@ -1341,6 +1341,16 @@ SILCloner<ImplClass>::visitCopyAddrInst(CopyAddrInst *Inst) {
Inst->isInitializationOfDest()));
}

template <typename ImplClass>
void SILCloner<ImplClass>::visitMarkUnresolvedMoveAddrInst(
MarkUnresolvedMoveAddrInst *Inst) {
getBuilder().setCurrentDebugScope(getOpScope(Inst->getDebugScope()));
auto *MVI = getBuilder().createMarkUnresolvedMoveAddr(
getOpLocation(Inst->getLoc()), getOpValue(Inst->getSrc()),
getOpValue(Inst->getDest()));
recordClonedInstruction(Inst, MVI);
}

template<typename ImplClass>
void
SILCloner<ImplClass>::visitBindMemoryInst(BindMemoryInst *Inst) {
Expand Down
48 changes: 47 additions & 1 deletion include/swift/SIL/SILInstruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -1793,6 +1793,10 @@ struct SILDebugVariable {
Implicit == V.Implicit && Type == V.Type && Loc == V.Loc &&
Scope == V.Scope;
}

bool isLet() const { return Name.size() && Constant; }

bool isVar() const { return Name.size() && !Constant; }
};

/// A DebugVariable where storage for the strings has been
Expand Down Expand Up @@ -2004,7 +2008,20 @@ class AllocStackInst final
auto VI = TailAllocatedDebugVariable(RawValue);
return VI.get(getDecl(), getTrailingObjects<char>(), AuxVarType, VarDeclLoc,
VarDeclScope, DIExprElements);
};
}

bool isLet() const {
if (auto varInfo = getVarInfo())
return varInfo->isLet();
return false;
}

bool isVar() const {
if (auto varInfo = getVarInfo())
return varInfo->isVar();
return false;
}

void setArgNo(unsigned N) {
auto RawValue = SILNode::Bits.AllocStackInst.VarInfo;
auto VI = TailAllocatedDebugVariable(RawValue);
Expand Down Expand Up @@ -7424,6 +7441,35 @@ class MoveValueInst
void setAllowsDiagnostics(bool newValue) { allowDiagnostics = newValue; }
};

/// Equivalent to a copy_addr to [init] except that it is used for diagnostics
/// and should not be pattern matched. During the diagnostic passes, the "move
/// function" checker for addresses always converts this to a copy_addr [init]
/// (if we emitted a diagnostic and proved we could not emit a move here) or a
/// copy_addr [take][init] if we can. So this should never occur in canonical
/// SIL.
class MarkUnresolvedMoveAddrInst
: public InstructionBase<SILInstructionKind::MarkUnresolvedMoveAddrInst,
NonValueInstruction>,
public CopyLikeInstruction {
friend class SILBuilder;

FixedOperandList<2> Operands;

MarkUnresolvedMoveAddrInst(SILDebugLocation DebugLoc, SILValue srcAddr,
SILValue takeAddr)
: InstructionBase(DebugLoc), Operands(this, srcAddr, takeAddr) {}

public:
SILValue getSrc() const { return Operands[Src].get(); }
SILValue getDest() const { return Operands[Dest].get(); }

void setSrc(SILValue V) { Operands[Src].set(V); }
void setDest(SILValue V) { Operands[Dest].set(V); }

ArrayRef<Operand> getAllOperands() const { return Operands.asArray(); }
MutableArrayRef<Operand> getAllOperands() { return Operands.asArray(); }
};

/// Given an object reference, return true iff it is non-nil and refers
/// to a native swift object with strong reference count of 1.
class IsUniqueInst
Expand Down
11 changes: 9 additions & 2 deletions include/swift/SIL/SILNodes.def
Original file line number Diff line number Diff line change
Expand Up @@ -591,8 +591,15 @@ ABSTRACT_VALUE_AND_INST(SingleValueInstruction, ValueBase, SILInstruction)
SingleValueInstruction, None, MayRelease)
// A move_value is an OSSA only instruction. Its result does not have any side
// effects relative to other OSSA values like copy_value.
SINGLE_VALUE_INST(MoveValueInst, move_value,
SingleValueInstruction, None, DoesNotRelease)
SINGLE_VALUE_INST(MoveValueInst, move_value, SingleValueInstruction, None,
DoesNotRelease)

// A move_addr is a Raw SIL only instruction that is equivalent to a copy_addr
// [init]. It is lowered during the diagnostic passes to a copy_addr [init] if
// the move checker found uses that prevented us from converting this to a
// move or if we do not find such uses, a copy_addr [init] [take].
NON_VALUE_INST(MarkUnresolvedMoveAddrInst, mark_unresolved_move_addr,
SILInstruction, None, DoesNotRelease)

// IsUnique does not actually write to memory but should be modeled
// as such. Its operand is a pointer to an object reference. The
Expand Down
3 changes: 3 additions & 0 deletions include/swift/SILOptimizer/PassManager/Passes.def
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,9 @@ PASS(MoveKillsCopyableValuesChecker, "sil-move-kills-copyable-values-checker",
"to _move do not have any uses later than the _move")
PASS(LexicalLifetimeEliminator, "sil-lexical-lifetime-eliminator",
"Pass that removes lexical lifetime markers from borrows and alloc stack")
PASS(MoveKillsCopyableAddressesChecker, "sil-move-kills-copyable-addresses-checker",
"Pass that checks that any copyable (non-move only) address that is passed "
"to _move do not have any uses later than the _move")
PASS(PruneVTables, "prune-vtables",
"Mark class methods that do not require vtable dispatch")
PASS_RANGE(AllPasses, AADumper, PruneVTables)
Expand Down
3 changes: 3 additions & 0 deletions lib/IRGen/IRGenSIL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1262,6 +1262,9 @@ class IRGenSILFunction :
void visitDeallocPartialRefInst(DeallocPartialRefInst *i);

void visitCopyAddrInst(CopyAddrInst *i);
void visitMarkUnresolvedMoveAddrInst(MarkUnresolvedMoveAddrInst *mai) {
llvm_unreachable("Valid only when ownership is enabled");
}
void visitDestroyAddrInst(DestroyAddrInst *i);

void visitBindMemoryInst(BindMemoryInst *i);
Expand Down
1 change: 1 addition & 0 deletions lib/SIL/IR/OperandOwnership.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ OPERAND_OWNERSHIP(TrivialUse, CheckedCastAddrBranch)
OPERAND_OWNERSHIP(TrivialUse, CondBranch)
OPERAND_OWNERSHIP(TrivialUse, CondFail)
OPERAND_OWNERSHIP(TrivialUse, CopyAddr)
OPERAND_OWNERSHIP(TrivialUse, MarkUnresolvedMoveAddr)
OPERAND_OWNERSHIP(TrivialUse, DeallocStack)
OPERAND_OWNERSHIP(TrivialUse, DeinitExistentialAddr)
OPERAND_OWNERSHIP(TrivialUse, DestroyAddr)
Expand Down
4 changes: 4 additions & 0 deletions lib/SIL/IR/SILInstruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1167,6 +1167,10 @@ bool SILInstruction::mayRelease() const {
return CopyAddr->isInitializationOfDest() ==
IsInitialization_t::IsNotInitialization;
}
// mark_unresolved_move_addr is equivalent to a copy_addr [init], so a release
// does not occur.
case SILInstructionKind::MarkUnresolvedMoveAddrInst:
return false;

case SILInstructionKind::BuiltinInst: {
auto *BI = cast<BuiltinInst>(this);
Expand Down
5 changes: 5 additions & 0 deletions lib/SIL/IR/SILPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1703,6 +1703,11 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
*this << getIDAndType(CI->getDest());
}

void visitMarkUnresolvedMoveAddrInst(MarkUnresolvedMoveAddrInst *CI) {
*this << Ctx.getID(CI->getSrc()) << " to ";
*this << getIDAndType(CI->getDest());
}

void visitBindMemoryInst(BindMemoryInst *BI) {
*this << getIDAndType(BI->getBase()) << ", ";
*this << getIDAndType(BI->getIndex()) << " to ";
Expand Down
28 changes: 28 additions & 0 deletions lib/SIL/Parser/ParseSIL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4771,6 +4771,34 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B,
IsInitialization_t(IsInit));
break;
}
case SILInstructionKind::MarkUnresolvedMoveAddrInst: {
UnresolvedValueName SrcLName;
SILValue DestLVal;
SourceLoc ToLoc, DestLoc;
Identifier ToToken;
if (parseValueName(SrcLName) ||
parseSILIdentifier(ToToken, ToLoc, diag::expected_tok_in_sil_instr,
"to") ||
parseTypedValueRef(DestLVal, DestLoc, B) ||
parseSILDebugLocation(InstLoc, B))
return true;

if (ToToken.str() != "to") {
P.diagnose(ToLoc, diag::expected_tok_in_sil_instr, "to");
return true;
}

if (!DestLVal->getType().isAddress()) {
P.diagnose(DestLoc, diag::sil_invalid_instr_operands);
return true;
}

SILValue SrcLVal =
getLocalValue(SrcLName, DestLVal->getType(), InstLoc, B);
ResultVal = B.createMarkUnresolvedMoveAddr(InstLoc, SrcLVal, DestLVal);
break;
}

case SILInstructionKind::BindMemoryInst: {
SILValue IndexVal;
SILType EltTy;
Expand Down
5 changes: 5 additions & 0 deletions lib/SIL/Utils/InstructionUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,11 @@ RuntimeEffect swift::getRuntimeEffect(SILInstruction *inst, SILType &impactType)
return RuntimeEffect::MetaData | RuntimeEffect::RefCounting;
return RuntimeEffect::MetaData;
}
// Equialent to a copy_addr [init]
case SILInstructionKind::MarkUnresolvedMoveAddrInst: {
return RuntimeEffect::MetaData | RuntimeEffect::RefCounting;
}

case SILInstructionKind::StoreInst:
switch (cast<StoreInst>(inst)->getOwnershipQualifier()) {
case StoreOwnershipQualifier::Unqualified:
Expand Down
8 changes: 8 additions & 0 deletions lib/SIL/Utils/MemAccessUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1904,6 +1904,9 @@ bool swift::memInstMustInitialize(Operand *memOper) {
auto *CAI = cast<CopyAddrInst>(memInst);
return CAI->getDest() == address && CAI->isInitializationOfDest();
}
case SILInstructionKind::MarkUnresolvedMoveAddrInst: {
return cast<MarkUnresolvedMoveAddrInst>(memInst)->getDest() == address;
}
case SILInstructionKind::InitExistentialAddrInst:
case SILInstructionKind::InitEnumDataAddrInst:
case SILInstructionKind::InjectEnumAddrInst:
Expand Down Expand Up @@ -2346,6 +2349,11 @@ void swift::visitAccessedAddress(SILInstruction *I,
visitor(&I->getAllOperands()[CopyAddrInst::Dest]);
return;

case SILInstructionKind::MarkUnresolvedMoveAddrInst:
visitor(&I->getAllOperands()[MarkUnresolvedMoveAddrInst::Src]);
visitor(&I->getAllOperands()[MarkUnresolvedMoveAddrInst::Dest]);
return;

#define NEVER_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...) \
case SILInstructionKind::Store##Name##Inst:
#include "swift/AST/ReferenceStorage.def"
Expand Down
5 changes: 5 additions & 0 deletions lib/SIL/Utils/MemoryLocations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,11 @@ bool MemoryLocations::analyzeLocationUsesRecursively(SILValue V, unsigned locIdx
case SILInstructionKind::SwitchEnumAddrInst:
case SILInstructionKind::WitnessMethodInst:
break;
case SILInstructionKind::MarkUnresolvedMoveAddrInst:
// We do not want the memory lifetime verifier to verify move_addr inst
// since the MarkUnresolvedMoveAddrChecker will validate that its uses
// are correct.
return false;
default:
return false;
}
Expand Down
23 changes: 12 additions & 11 deletions lib/SIL/Utils/OwnershipUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -701,17 +701,18 @@ swift::findTransitiveUsesForAddress(SILValue projectedAddress,
}
// First, eliminate "end point uses" that we just need to check liveness at
// and do not need to check transitive uses of.
if (isa<LoadInst>(user) || isa<CopyAddrInst>(user) || isIncidentalUse(user)
|| isa<StoreInst>(user) || isa<DestroyAddrInst>(user)
|| isa<AssignInst>(user) || isa<YieldInst>(user)
|| isa<LoadUnownedInst>(user) || isa<StoreUnownedInst>(user)
|| isa<EndApplyInst>(user) || isa<LoadWeakInst>(user)
|| isa<StoreWeakInst>(user) || isa<AssignByWrapperInst>(user)
|| isa<BeginUnpairedAccessInst>(user)
|| isa<EndUnpairedAccessInst>(user) || isa<WitnessMethodInst>(user)
|| isa<SwitchEnumAddrInst>(user) || isa<CheckedCastAddrBranchInst>(user)
|| isa<SelectEnumAddrInst>(user) || isa<InjectEnumAddrInst>(user)
|| isa<IsUniqueInst>(user)) {
if (isa<LoadInst>(user) || isa<CopyAddrInst>(user) ||
isa<MarkUnresolvedMoveAddrInst>(user) || isIncidentalUse(user) ||
isa<StoreInst>(user) || isa<DestroyAddrInst>(user) ||
isa<AssignInst>(user) || isa<YieldInst>(user) ||
isa<LoadUnownedInst>(user) || isa<StoreUnownedInst>(user) ||
isa<EndApplyInst>(user) || isa<LoadWeakInst>(user) ||
isa<StoreWeakInst>(user) || isa<AssignByWrapperInst>(user) ||
isa<BeginUnpairedAccessInst>(user) ||
isa<EndUnpairedAccessInst>(user) || isa<WitnessMethodInst>(user) ||
isa<SwitchEnumAddrInst>(user) || isa<CheckedCastAddrBranchInst>(user) ||
isa<SelectEnumAddrInst>(user) || isa<InjectEnumAddrInst>(user) ||
isa<IsUniqueInst>(user)) {
leafUse(op);
continue;
}
Expand Down
11 changes: 11 additions & 0 deletions lib/SIL/Verifier/LoadBorrowImmutabilityChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,17 @@ bool GatherWritesVisitor::visitUse(Operand *op, AccessUseType useTy) {
}
return true;

case SILInstructionKind::MarkUnresolvedMoveAddrInst:
if (cast<MarkUnresolvedMoveAddrInst>(user)->getDest() == op->get()) {
writeAccumulator.push_back(op);
return true;
}

// This operand is the move source, we just return true. This is because
// mark_unresolved_move_addr semantically is treated as a copy_addr of the
// source. The checker determines if we can convert it to a move.
return true;

// If this value is dependent on another, conservatively consider it a write.
//
// FIXME: explain why a mark_dependence effectively writes to storage.
Expand Down
8 changes: 8 additions & 0 deletions lib/SIL/Verifier/MemoryLifetimeVerifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,14 @@ void MemoryLifetimeVerifier::initDataflowInBlock(SILBasicBlock *block,
genBits(state, CAI->getDest());
break;
}
case SILInstructionKind::MarkUnresolvedMoveAddrInst: {
auto *MMAI = cast<MarkUnresolvedMoveAddrInst>(&I);
// We do not treat the move addr inst as invalidating its src since we
// are going to prove that we do not inappropriately reuse the memory
// later.
genBits(state, MMAI->getDest());
break;
}
case SILInstructionKind::InjectEnumAddrInst: {
auto *IEAI = cast<InjectEnumAddrInst>(&I);
int enumIdx = locations.getLocationIdx(IEAI->getOperand());
Expand Down
17 changes: 17 additions & 0 deletions lib/SIL/Verifier/SILVerifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,11 @@ struct ImmutableAddressUseVerifier {
break;
case SILInstructionKind::EndAccessInst:
break;
case SILInstructionKind::MarkUnresolvedMoveAddrInst:
// We model mark_unresolved_move_addr as a copy_addr [init]. So no
// mutation can happen. The checker will prove eventually that we can
// convert it to a copy_addr [take] [init].
break;
case SILInstructionKind::CopyAddrInst:
if (isConsumingOrMutatingCopyAddrUse(use))
return true;
Expand Down Expand Up @@ -2637,6 +2642,18 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
"cannot directly copy type with inaccessible ABI");
}

void checkMarkUnresolvedMoveAddrInst(MarkUnresolvedMoveAddrInst *SI) {
require(F.hasOwnership(), "Only valid in OSSA.");
require(F.getModule().getStage() == SILStage::Raw, "Only valid in Raw SIL");
require(SI->getSrc()->getType().isAddress(), "Src value should be lvalue");
require(SI->getDest()->getType().isAddress(),
"Dest address should be lvalue");
requireSameType(SI->getDest()->getType(), SI->getSrc()->getType(),
"Store operand type and dest type mismatch");
require(F.isTypeABIAccessible(SI->getDest()->getType()),
"cannot directly copy type with inaccessible ABI");
}

void checkRetainValueInst(RetainValueInst *I) {
require(I->getOperand()->getType().isObject(),
"Source value should be an object value");
Expand Down
5 changes: 5 additions & 0 deletions lib/SILGen/SILGenLazyConformance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ class LazyConformanceEmitter : public SILInstructionVisitor<LazyConformanceEmitt
SGM.useConformancesFromType(CAI->getDest()->getType().getASTType());
}

void visitMarkUnresolvedMoveAddrInst(MarkUnresolvedMoveAddrInst *MAI) {
SGM.useConformancesFromType(MAI->getSrc()->getType().getASTType());
SGM.useConformancesFromType(MAI->getDest()->getType().getASTType());
}

void visitCopyValueInst(CopyValueInst *CVI) {
SGM.useConformancesFromType(CVI->getOperand()->getType().getASTType());
}
Expand Down
Loading