Skip to content

Commit 3cc98eb

Browse files
eecksteineeckstein
committed
MemoryLifetimeVerifier: be more precise with indirect function arguments.
Optimizations can rely on alias analysis to know that an in-argument (or parts of it) is not actually read. We have to do the same in the verifier: if alias analysis says that an in-argument is not read, there is no need that the memory location is initialized. Fixes a false verifier error. rdar://106806899
1 parent 48b2328 commit 3cc98eb

File tree

12 files changed

+105
-39
lines changed

12 files changed

+105
-39
lines changed

‎include/swift/SIL/SILFunction.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ class SILFunctionBuilder;
3838
class SILProfiler;
3939
class BasicBlockBitfield;
4040
class NodeBitfield;
41+
class SILPassManager;
4142

4243
namespace Lowering {
4344
class TypeLowering;
@@ -1410,18 +1411,19 @@ class SILFunction
14101411

14111412
/// verify - Run the SIL verifier to make sure that the SILFunction follows
14121413
/// invariants.
1413-
void verify(bool SingleFunction = true,
1414+
void verify(SILPassManager *passManager = nullptr,
1415+
bool SingleFunction = true,
14141416
bool isCompleteOSSA = true,
14151417
bool checkLinearLifetime = true) const;
14161418

14171419
/// Run the SIL verifier without assuming OSSA lifetimes end at dead end
14181420
/// blocks.
14191421
void verifyIncompleteOSSA() const {
1420-
verify(/*SingleFunction=*/true, /*completeOSSALifetimes=*/false);
1422+
verify(/*passManager*/nullptr, /*SingleFunction=*/true, /*completeOSSALifetimes=*/false);
14211423
}
14221424

14231425
/// Verifies the lifetime of memory locations in the function.
1424-
void verifyMemoryLifetime();
1426+
void verifyMemoryLifetime(SILPassManager *passManager);
14251427

14261428
/// Run the SIL ownership verifier to check that all values with ownership
14271429
/// have a linear lifetime. Regular OSSA invariants are checked separately in

‎include/swift/SIL/SILModule.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -902,9 +902,13 @@ class SILModule {
902902
/// fetched in the given module?
903903
bool isTypeMetadataForLayoutAccessible(SILType type);
904904

905+
void verify(bool isCompleteOSSA = true,
906+
bool checkLinearLifetime = true) const;
907+
905908
/// Run the SIL verifier to make sure that all Functions follow
906909
/// invariants.
907-
void verify(bool isCompleteOSSA = true,
910+
void verify(SILPassManager *passManager,
911+
bool isCompleteOSSA = true,
908912
bool checkLinearLifetime = true) const;
909913

910914
/// Run the SIL verifier without assuming OSSA lifetimes end at dead end

‎include/swift/SILOptimizer/PassManager/PassManager.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,12 +221,11 @@ class SILPassManager {
221221

222222
std::chrono::nanoseconds totalPassRuntime = std::chrono::nanoseconds(0);
223223

224+
public:
224225
/// C'tor. It creates and registers all analysis passes, which are defined
225-
/// in Analysis.def. This is private as it should only be used by
226-
/// ExecuteSILPipelineRequest.
226+
/// in Analysis.def.
227227
SILPassManager(SILModule *M, bool isMandatory, irgen::IRGenModule *IRMod);
228228

229-
public:
230229
const SILOptions &getOptions() const;
231230

232231
/// Searches for an analysis of type T in the list of registered

‎lib/SIL/Parser/ParseSIL.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,11 @@ ParseSILModuleRequest::evaluate(Evaluator &evaluator,
117117
"Failed to parse SIL but did not emit any errors!");
118118
return SILModule::createEmptyModule(desc.context, desc.conv, desc.opts);
119119
}
120+
// If SIL parsing succeeded, verify the generated SIL.
121+
if (!parser.Diags.hadAnyError() && !DisableInputVerify) {
122+
silMod->verify(/*SingleFunction=*/true, !ParseIncompleteOSSA);
123+
}
124+
120125
return silMod;
121126
}
122127

@@ -7046,10 +7051,6 @@ bool SILParserState::parseDeclSIL(Parser &P) {
70467051
if (FunctionState.diagnoseProblems())
70477052
return true;
70487053

7049-
// If SIL parsing succeeded, verify the generated SIL.
7050-
if (!P.Diags.hadAnyError() && !DisableInputVerify)
7051-
FunctionState.F->verify(/*SingleFunction=*/true, !ParseIncompleteOSSA);
7052-
70537054
return false;
70547055
}
70557056

‎lib/SIL/Verifier/MemoryLifetimeVerifier.cpp

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@
1717
#include "swift/SIL/SILFunction.h"
1818
#include "swift/SIL/ApplySite.h"
1919
#include "swift/SIL/BasicBlockDatastructures.h"
20+
#include "swift/SILOptimizer/Analysis/AliasAnalysis.h"
21+
#include "swift/SILOptimizer/PassManager/PassManager.h"
2022
#include "llvm/Support/CommandLine.h"
2123

2224
using namespace swift;
@@ -40,6 +42,7 @@ class MemoryLifetimeVerifier {
4042
using BlockState = BitDataflow::BlockState;
4143

4244
SILFunction *function;
45+
AliasAnalysis *aliasAnalysis;
4346
MemoryLocations locations;
4447

4548
/// alloc_stack memory locations which are used for store_borrow.
@@ -77,6 +80,8 @@ class MemoryLifetimeVerifier {
7780
/// \p addr, are set in \p bits.
7881
void requireBitsSet(const Bits &bits, SILValue addr, SILInstruction *where);
7982

83+
void requireBitsSetForArgument(const Bits &bits, SILValue addr, SILInstruction *applyInst);
84+
8085
bool isStoreBorrowLocation(SILValue addr) {
8186
auto *loc = locations.getLocation(addr);
8287
return loc && storeBorrowLocations.anyCommon(loc->subLocations);
@@ -132,9 +137,12 @@ class MemoryLifetimeVerifier {
132137
}
133138

134139
public:
135-
MemoryLifetimeVerifier(SILFunction *function) :
136-
function(function), locations(/*handleNonTrivialProjections*/ true,
137-
/*handleTrivialLocations*/ true) {}
140+
MemoryLifetimeVerifier(SILFunction *function, SILPassManager *passManager) :
141+
function(function),
142+
aliasAnalysis(passManager ? passManager->getAnalysis<AliasAnalysis>(function)
143+
: nullptr),
144+
locations(/*handleNonTrivialProjections*/ true,
145+
/*handleTrivialLocations*/ true) {}
138146

139147
/// The main entry point to verify the lifetime of all memory locations in
140148
/// the function.
@@ -274,6 +282,30 @@ void MemoryLifetimeVerifier::requireBitsSet(const Bits &bits, SILValue addr,
274282
}
275283
}
276284

285+
void MemoryLifetimeVerifier::requireBitsSetForArgument(const Bits &bits, SILValue addr,
286+
SILInstruction *applyInst) {
287+
// Optimizations can rely on alias analysis to know that an in-argument (or
288+
// parts of it) is not actually read.
289+
// We have to do the same in the verifier: if alias analysis says that an in-
290+
// argument is not read, there is no need that the memory location is initialized.
291+
292+
// Not all calls to the verifier provide the alias analysis.
293+
if (!aliasAnalysis)
294+
return;
295+
296+
if (auto *loc = locations.getLocation(addr)) {
297+
Bits missingBits = ~bits & loc->subLocations;
298+
for (int errorLocIdx = missingBits.find_first(); errorLocIdx >= 0;
299+
errorLocIdx = missingBits.find_next(errorLocIdx)) {
300+
auto *errorLoc = locations.getLocation(errorLocIdx);
301+
if (aliasAnalysis->mayReadFromMemory(applyInst, errorLoc->representativeValue)) {
302+
reportError("argument memory is not initialized, but should be",
303+
errorLocIdx, applyInst);
304+
}
305+
}
306+
}
307+
}
308+
277309
void MemoryLifetimeVerifier::requireNoStoreBorrowLocation(
278310
SILValue addr, SILInstruction *where) {
279311
if (isa<StoreBorrowInst>(addr)) {
@@ -815,7 +847,7 @@ void MemoryLifetimeVerifier::checkFuncArgument(Bits &bits, Operand &argumentOp,
815847

816848
switch (argumentConvention) {
817849
case SILArgumentConvention::Indirect_In:
818-
requireBitsSet(bits, argumentOp.get(), applyInst);
850+
requireBitsSetForArgument(bits, argumentOp.get(), applyInst);
819851
locations.clearBits(bits, argumentOp.get());
820852
break;
821853
case SILArgumentConvention::Indirect_Out:
@@ -825,7 +857,7 @@ void MemoryLifetimeVerifier::checkFuncArgument(Bits &bits, Operand &argumentOp,
825857
break;
826858
case SILArgumentConvention::Indirect_In_Guaranteed:
827859
case SILArgumentConvention::Indirect_Inout:
828-
requireBitsSet(bits, argumentOp.get(), applyInst);
860+
requireBitsSetForArgument(bits, argumentOp.get(), applyInst);
829861
break;
830862
case SILArgumentConvention::Indirect_InoutAliasable:
831863
// We don't require any locations to be initialized for a partial_apply
@@ -834,7 +866,7 @@ void MemoryLifetimeVerifier::checkFuncArgument(Bits &bits, Operand &argumentOp,
834866
// closures capture the whole "self". When this is done in an initializer
835867
// it can happen that not all fields of "self" are initialized, yet.
836868
if (!isa<PartialApplyInst>(applyInst))
837-
requireBitsSet(bits, argumentOp.get(), applyInst);
869+
requireBitsSetForArgument(bits, argumentOp.get(), applyInst);
838870
break;
839871
case SILArgumentConvention::Direct_Owned:
840872
case SILArgumentConvention::Direct_Unowned:
@@ -870,7 +902,7 @@ void MemoryLifetimeVerifier::verify() {
870902

871903
} // anonymous namespace
872904

873-
void SILFunction::verifyMemoryLifetime() {
874-
MemoryLifetimeVerifier verifier(this);
905+
void SILFunction::verifyMemoryLifetime(SILPassManager *passManager) {
906+
MemoryLifetimeVerifier verifier(this, passManager);
875907
verifier.verify();
876908
}

‎lib/SIL/Verifier/SILVerifier.cpp

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -724,6 +724,7 @@ static void checkAddressWalkerCanVisitAllTransitiveUses(SILValue address) {
724724
class SILVerifier : public SILVerifierBase<SILVerifier> {
725725
ModuleDecl *M;
726726
const SILFunction &F;
727+
SILPassManager *passManager;
727728
SILFunctionConventions fnConv;
728729
Lowering::TypeConverter &TC;
729730

@@ -993,8 +994,10 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
993994
return numInsts;
994995
}
995996

996-
SILVerifier(const SILFunction &F, bool SingleFunction, bool checkLinearLifetime)
997+
SILVerifier(const SILFunction &F, SILPassManager *passManager,
998+
bool SingleFunction, bool checkLinearLifetime)
997999
: M(F.getModule().getSwiftModule()), F(F),
1000+
passManager(passManager),
9981001
fnConv(F.getConventionsInContext()), TC(F.getModule().Types),
9991002
SingleFunction(SingleFunction),
10001003
checkLinearLifetime(checkLinearLifetime),
@@ -6523,7 +6526,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
65236526

65246527
if (F->hasOwnership() && F->shouldVerifyOwnership() &&
65256528
!mod.getASTContext().hadError()) {
6526-
F->verifyMemoryLifetime();
6529+
F->verifyMemoryLifetime(passManager);
65276530
}
65286531
}
65296532

@@ -6563,22 +6566,25 @@ static bool verificationEnabled(const SILModule &M) {
65636566

65646567
/// verify - Run the SIL verifier to make sure that the SILFunction follows
65656568
/// invariants.
6566-
void SILFunction::verify(bool SingleFunction, bool isCompleteOSSA,
6569+
void SILFunction::verify(SILPassManager *passManager,
6570+
bool SingleFunction, bool isCompleteOSSA,
65676571
bool checkLinearLifetime) const {
65686572
if (!verificationEnabled(getModule()))
65696573
return;
65706574

65716575
// Please put all checks in visitSILFunction in SILVerifier, not here. This
65726576
// ensures that the pretty stack trace in the verifier is included with the
65736577
// back trace when the verifier crashes.
6574-
SILVerifier(*this, SingleFunction, checkLinearLifetime).verify(isCompleteOSSA);
6578+
SILVerifier verifier(*this, passManager, SingleFunction, checkLinearLifetime);
6579+
verifier.verify(isCompleteOSSA);
65756580
}
65766581

65776582
void SILFunction::verifyCriticalEdges() const {
65786583
if (!verificationEnabled(getModule()))
65796584
return;
65806585

6581-
SILVerifier(*this, /*SingleFunction=*/true,
6586+
SILVerifier(*this, /*passManager=*/nullptr,
6587+
/*SingleFunction=*/true,
65826588
/*checkLinearLifetime=*/ false).verifyBranches(this);
65836589
}
65846590

@@ -6696,7 +6702,8 @@ void SILVTable::verify(const SILModule &M) const {
66966702
}
66976703

66986704
if (M.getStage() != SILStage::Lowered) {
6699-
SILVerifier(*entry.getImplementation(), /*SingleFunction=*/true,
6705+
SILVerifier(*entry.getImplementation(), /*passManager=*/nullptr,
6706+
/*SingleFunction=*/true,
67006707
/*checkLinearLifetime=*/ false)
67016708
.requireABICompatibleFunctionTypes(
67026709
baseInfo.getSILType().castTo<SILFunctionType>(),
@@ -6841,8 +6848,14 @@ void SILGlobalVariable::verify() const {
68416848
}
68426849
}
68436850

6844-
/// Verify the module.
68456851
void SILModule::verify(bool isCompleteOSSA, bool checkLinearLifetime) const {
6852+
SILPassManager pm(const_cast<SILModule *>(this), /*isMandatory=*/false, /*IRMod=*/nullptr);
6853+
verify(&pm, isCompleteOSSA, checkLinearLifetime);
6854+
}
6855+
6856+
/// Verify the module.
6857+
void SILModule::verify(SILPassManager *passManager,
6858+
bool isCompleteOSSA, bool checkLinearLifetime) const {
68466859
if (!verificationEnabled(*this))
68476860
return;
68486861

@@ -6857,7 +6870,7 @@ void SILModule::verify(bool isCompleteOSSA, bool checkLinearLifetime) const {
68576870
llvm::errs() << "Symbol redefined: " << f.getName() << "!\n";
68586871
assert(false && "triggering standard assertion failure routine");
68596872
}
6860-
f.verify(/*singleFunction*/ false, isCompleteOSSA, checkLinearLifetime);
6873+
f.verify(passManager, /*singleFunction*/ false, isCompleteOSSA, checkLinearLifetime);
68616874
}
68626875

68636876
// Check all globals.

‎lib/SILOptimizer/LoopTransforms/ArrayBoundsCheckOpts.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1271,7 +1271,7 @@ bool ABCOpt::processLoop(SILLoop *Loop) {
12711271
Changed |= hoistChecksInLoop(DT->getNode(Header), ABC, IndVars, Preheader,
12721272
Header, SingleExitingBlk, /*recursionDepth*/ 0);
12731273
if (Changed) {
1274-
Preheader->getParent()->verify();
1274+
Preheader->getParent()->verify(getPassManager());
12751275
}
12761276
return Changed;
12771277
}

‎lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1251,7 +1251,7 @@ class ClosureLifetimeFixup : public SILFunctionTransform {
12511251
}
12521252
invalidateAnalysis(analysisInvalidationKind(modifiedCFG));
12531253
}
1254-
LLVM_DEBUG(getFunction()->verify());
1254+
LLVM_DEBUG(getFunction()->verify(getPassManager()));
12551255

12561256
}
12571257

‎lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -751,7 +751,7 @@ struct OwnershipModelEliminator : SILFunctionTransform {
751751
"Found verification error when verifying before lowering "
752752
"ownership. Please re-run with -sil-verify-all to identify the "
753753
"actual pass that introduced the verification error.");
754-
f->verify();
754+
f->verify(getPassManager());
755755
}
756756

757757
if (stripOwnership(*f)) {

‎lib/SILOptimizer/PassManager/PassManager.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -561,7 +561,7 @@ void SILPassManager::runPassOnFunction(unsigned TransIdx, SILFunction *F) {
561561
(SILVerifyAroundPass.end() != std::find_if(SILVerifyAroundPass.begin(),
562562
SILVerifyAroundPass.end(),
563563
MatchFun))) {
564-
F->verify();
564+
F->verify(this);
565565
verifyAnalyses();
566566
}
567567

@@ -656,7 +656,7 @@ void SILPassManager::runPassOnFunction(unsigned TransIdx, SILFunction *F) {
656656

657657
if (getOptions().VerifyAll &&
658658
(CurrentPassHasInvalidated || SILVerifyWithoutInvalidation)) {
659-
F->verify();
659+
F->verify(this);
660660
verifyAnalyses(F);
661661
} else {
662662
if ((SILVerifyAfterPass.end() != std::find_if(SILVerifyAfterPass.begin(),
@@ -665,7 +665,7 @@ void SILPassManager::runPassOnFunction(unsigned TransIdx, SILFunction *F) {
665665
(SILVerifyAroundPass.end() != std::find_if(SILVerifyAroundPass.begin(),
666666
SILVerifyAroundPass.end(),
667667
MatchFun))) {
668-
F->verify();
668+
F->verify(this);
669669
verifyAnalyses();
670670
}
671671
}
@@ -771,7 +771,7 @@ void SILPassManager::runModulePass(unsigned TransIdx) {
771771
(SILVerifyAroundPass.end() != std::find_if(SILVerifyAroundPass.begin(),
772772
SILVerifyAroundPass.end(),
773773
MatchFun))) {
774-
Mod->verify();
774+
Mod->verify(this);
775775
verifyAnalyses();
776776
}
777777

@@ -805,7 +805,7 @@ void SILPassManager::runModulePass(unsigned TransIdx) {
805805

806806
if (Options.VerifyAll &&
807807
(CurrentPassHasInvalidated || !SILVerifyWithoutInvalidation)) {
808-
Mod->verify();
808+
Mod->verify(this);
809809
verifyAnalyses();
810810
} else {
811811
if ((SILVerifyAfterPass.end() != std::find_if(SILVerifyAfterPass.begin(),
@@ -814,7 +814,7 @@ void SILPassManager::runModulePass(unsigned TransIdx) {
814814
(SILVerifyAroundPass.end() != std::find_if(SILVerifyAroundPass.begin(),
815815
SILVerifyAroundPass.end(),
816816
MatchFun))) {
817-
Mod->verify();
817+
Mod->verify(this);
818818
verifyAnalyses();
819819
}
820820
}
@@ -967,7 +967,7 @@ void SILPassManager::addFunctionToWorklist(SILFunction *F,
967967
// this function to the pass manager to ensure that we perform this
968968
// verification.
969969
if (getOptions().VerifyAll) {
970-
F->verify();
970+
F->verify(this);
971971
}
972972

973973
NewLevel = DerivationLevels[DerivedFrom] + 1;

‎lib/SILOptimizer/Transforms/EagerSpecializer.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -884,7 +884,7 @@ void EagerSpecializerTransform::run() {
884884
[&](const SILSpecializeAttr *SA, SILFunction *NewFunc,
885885
const ReabstractionInfo &ReInfo) {
886886
if (NewFunc) {
887-
NewFunc->verify();
887+
NewFunc->verify(getPassManager());
888888
Changed = true;
889889
EagerDispatch(&F, ReInfo).emitDispatchTo(NewFunc);
890890
}
@@ -904,7 +904,7 @@ void EagerSpecializerTransform::run() {
904904
// If any specializations were created, reverify the original body now that it
905905
// has checks.
906906
if (!newFunctions.empty())
907-
F.verify();
907+
F.verify(getPassManager());
908908

909909
for (SILFunction *newF : newFunctions) {
910910
addFunctionToPassManagerWorklist(newF, nullptr);

0 commit comments

Comments
 (0)