Merge pull request #65592 from eeckstein/fix-sil-verifier

MemoryLifetimeVerifier: be more precise with indirect function arguments.

Author: eeckstein

SwiftCompilerSources/Sources/Optimizer/Analysis/CalleeAnalysis.swift

@@ -23,10 +23,9 @@ public struct CalleeAnalysis {
         return inst.instruction.isDeinitBarrier(bca.analysis)
       },
       // getMemBehaviorFn
-      { (bridgedCtxt: BridgedPassContext, bridgedApply: BridgedInstruction, observeRetains: Bool) -> swift.MemoryBehavior in
-        let context = FunctionPassContext(_bridged: bridgedCtxt)
+      { (bridgedApply: BridgedInstruction, observeRetains: Bool, bca: BridgedCalleeAnalysis) -> swift.MemoryBehavior in
         let apply = bridgedApply.instruction as! ApplySite
-        let e = context.calleeAnalysis.getSideEffects(of: apply)
+        let e = bca.analysis.getSideEffects(of: apply)
         return e.getMemBehavior(observeRetains: observeRetains)
       }
     )

include/swift/SIL/SILFunction.h

@@ -38,6 +38,7 @@ class SILFunctionBuilder;
 class SILProfiler;
 class BasicBlockBitfield;
 class NodeBitfield;
+class SILPassManager;
 
 namespace Lowering {
 class TypeLowering;
@@ -1410,18 +1411,19 @@ class SILFunction
 
   /// verify - Run the SIL verifier to make sure that the SILFunction follows
   /// invariants.
-  void verify(bool SingleFunction = true,
+  void verify(SILPassManager *passManager = nullptr,
+              bool SingleFunction = true,
               bool isCompleteOSSA = true,
               bool checkLinearLifetime = true) const;
 
   /// Run the SIL verifier without assuming OSSA lifetimes end at dead end
   /// blocks.
   void verifyIncompleteOSSA() const {
-    verify(/*SingleFunction=*/true, /*completeOSSALifetimes=*/false);
+    verify(/*passManager*/nullptr, /*SingleFunction=*/true, /*completeOSSALifetimes=*/false);
   }
 
   /// Verifies the lifetime of memory locations in the function.
-  void verifyMemoryLifetime();
+  void verifyMemoryLifetime(SILPassManager *passManager);
 
   /// Run the SIL ownership verifier to check that all values with ownership
   /// have a linear lifetime. Regular OSSA invariants are checked separately in

include/swift/SIL/SILModule.h

@@ -902,9 +902,13 @@ class SILModule {
   /// fetched in the given module?
   bool isTypeMetadataForLayoutAccessible(SILType type);
 
+  void verify(bool isCompleteOSSA = true,
+              bool checkLinearLifetime = true) const;
+
   /// Run the SIL verifier to make sure that all Functions follow
   /// invariants.
-  void verify(bool isCompleteOSSA = true,
+  void verify(SILPassManager *passManager,
+              bool isCompleteOSSA = true,
               bool checkLinearLifetime = true) const;
 
   /// Run the SIL verifier without assuming OSSA lifetimes end at dead end

include/swift/SILOptimizer/Analysis/BasicCalleeAnalysis.h

@@ -183,7 +183,6 @@ class CalleeCache {
 
 class BasicCalleeAnalysis : public SILAnalysis {
   SILModule &M;
-  SILPassManager *pm = nullptr;
   std::unique_ptr<CalleeCache> Cache;
 
 public:
@@ -197,8 +196,6 @@ class BasicCalleeAnalysis : public SILAnalysis {
     return S->getKind() == SILAnalysisKind::BasicCallee;
   }
 
-  virtual void initialize(SILPassManager *pm) override { this->pm = pm; }
-
   /// Invalidate all information in this analysis.
   virtual void invalidate() override {
     Cache.reset();

include/swift/SILOptimizer/OptimizerBridging.h

@@ -62,7 +62,7 @@ struct BridgedCalleeAnalysis {
 
   typedef bool (* _Nonnull IsDeinitBarrierFn)(BridgedInstruction, BridgedCalleeAnalysis bca);
   typedef swift::MemoryBehavior (* _Nonnull GetMemBehvaiorFn)(
-        BridgedPassContext context, BridgedInstruction apply, bool observeRetains);
+        BridgedInstruction apply, bool observeRetains, BridgedCalleeAnalysis bca);
 
   static void registerAnalysis(IsDeinitBarrierFn isDeinitBarrierFn,
                                GetMemBehvaiorFn getEffectsFn);

include/swift/SILOptimizer/PassManager/PassManager.h

@@ -221,12 +221,11 @@ class SILPassManager {
 
   std::chrono::nanoseconds totalPassRuntime = std::chrono::nanoseconds(0);
 
+public:
   /// C'tor. It creates and registers all analysis passes, which are defined
-  /// in Analysis.def. This is private as it should only be used by
-  /// ExecuteSILPipelineRequest.
+  /// in Analysis.def.
   SILPassManager(SILModule *M, bool isMandatory, irgen::IRGenModule *IRMod);
 
-public:
   const SILOptions &getOptions() const;
 
   /// Searches for an analysis of type T in the list of registered

lib/SIL/Parser/ParseSIL.cpp

@@ -117,6 +117,11 @@ ParseSILModuleRequest::evaluate(Evaluator &evaluator,
            "Failed to parse SIL but did not emit any errors!");
     return SILModule::createEmptyModule(desc.context, desc.conv, desc.opts);
   }
+  // If SIL parsing succeeded, verify the generated SIL.
+  if (!parser.Diags.hadAnyError() && !DisableInputVerify) {
+    silMod->verify(/*SingleFunction=*/true, !ParseIncompleteOSSA);
+  }
+
   return silMod;
 }
 
@@ -7046,10 +7051,6 @@ bool SILParserState::parseDeclSIL(Parser &P) {
   if (FunctionState.diagnoseProblems())
     return true;
 
-  // If SIL parsing succeeded, verify the generated SIL.
-  if (!P.Diags.hadAnyError() && !DisableInputVerify)
-    FunctionState.F->verify(/*SingleFunction=*/true, !ParseIncompleteOSSA);
-
   return false;
 }
 

lib/SIL/Verifier/MemoryLifetimeVerifier.cpp

@@ -17,6 +17,8 @@
 #include "swift/SIL/SILFunction.h"
 #include "swift/SIL/ApplySite.h"
 #include "swift/SIL/BasicBlockDatastructures.h"
+#include "swift/SILOptimizer/Analysis/AliasAnalysis.h"
+#include "swift/SILOptimizer/PassManager/PassManager.h"
 #include "llvm/Support/CommandLine.h"
 
 using namespace swift;
@@ -40,6 +42,7 @@ class MemoryLifetimeVerifier {
   using BlockState = BitDataflow::BlockState;
 
   SILFunction *function;
+  AliasAnalysis *aliasAnalysis;
   MemoryLocations locations;
 
   /// alloc_stack memory locations which are used for store_borrow.
@@ -77,6 +80,8 @@ class MemoryLifetimeVerifier {
   /// \p addr, are set in \p bits.
   void requireBitsSet(const Bits &bits, SILValue addr, SILInstruction *where);
 
+  void requireBitsSetForArgument(const Bits &bits, SILValue addr, SILInstruction *applyInst);
+
   bool isStoreBorrowLocation(SILValue addr) {
     auto *loc = locations.getLocation(addr);
     return loc && storeBorrowLocations.anyCommon(loc->subLocations);
@@ -132,9 +137,12 @@ class MemoryLifetimeVerifier {
   }
 
 public:
-  MemoryLifetimeVerifier(SILFunction *function) :
-    function(function), locations(/*handleNonTrivialProjections*/ true,
-                                  /*handleTrivialLocations*/ true) {}
+  MemoryLifetimeVerifier(SILFunction *function, SILPassManager *passManager) :
+    function(function),
+    aliasAnalysis(passManager ? passManager->getAnalysis<AliasAnalysis>(function)
+                              : nullptr),
+    locations(/*handleNonTrivialProjections*/ true,
+              /*handleTrivialLocations*/ true) {}
 
   /// The main entry point to verify the lifetime of all memory locations in
   /// the function.
@@ -274,6 +282,30 @@ void MemoryLifetimeVerifier::requireBitsSet(const Bits &bits, SILValue addr,
   }
 }
 
+void MemoryLifetimeVerifier::requireBitsSetForArgument(const Bits &bits, SILValue addr,
+                                           SILInstruction *applyInst) {
+  // 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.
+
+  // Not all calls to the verifier provide the alias analysis.
+  if (!aliasAnalysis)
+    return;
+
+  if (auto *loc = locations.getLocation(addr)) {
+    Bits missingBits = ~bits & loc->subLocations;
+    for (int errorLocIdx = missingBits.find_first(); errorLocIdx >= 0;
+         errorLocIdx = missingBits.find_next(errorLocIdx)) {
+      auto *errorLoc = locations.getLocation(errorLocIdx);
+      if (aliasAnalysis->mayReadFromMemory(applyInst, errorLoc->representativeValue)) {
+        reportError("memory is not initialized, but should be",
+                    errorLocIdx, applyInst);
+      }
+    }
+  }
+}
+
 void MemoryLifetimeVerifier::requireNoStoreBorrowLocation(
     SILValue addr, SILInstruction *where) {
   if (isa<StoreBorrowInst>(addr)) {
@@ -815,7 +847,7 @@ void MemoryLifetimeVerifier::checkFuncArgument(Bits &bits, Operand &argumentOp,
 
   switch (argumentConvention) {
     case SILArgumentConvention::Indirect_In:
-      requireBitsSet(bits, argumentOp.get(), applyInst);
+      requireBitsSetForArgument(bits, argumentOp.get(), applyInst);
       locations.clearBits(bits, argumentOp.get());
       break;
     case SILArgumentConvention::Indirect_Out:
@@ -825,7 +857,7 @@ void MemoryLifetimeVerifier::checkFuncArgument(Bits &bits, Operand &argumentOp,
       break;
     case SILArgumentConvention::Indirect_In_Guaranteed:
     case SILArgumentConvention::Indirect_Inout:
-      requireBitsSet(bits, argumentOp.get(), applyInst);
+      requireBitsSetForArgument(bits, argumentOp.get(), applyInst);
       break;
     case SILArgumentConvention::Indirect_InoutAliasable:
       // We don't require any locations to be initialized for a partial_apply
@@ -834,7 +866,7 @@ void MemoryLifetimeVerifier::checkFuncArgument(Bits &bits, Operand &argumentOp,
       // closures capture the whole "self". When this is done in an initializer
       // it can happen that not all fields of "self" are initialized, yet.
       if (!isa<PartialApplyInst>(applyInst))
-        requireBitsSet(bits, argumentOp.get(), applyInst);
+        requireBitsSetForArgument(bits, argumentOp.get(), applyInst);
       break;
     case SILArgumentConvention::Direct_Owned:
     case SILArgumentConvention::Direct_Unowned:
@@ -870,7 +902,7 @@ void MemoryLifetimeVerifier::verify() {
 
 } // anonymous namespace
 
-void SILFunction::verifyMemoryLifetime() {
-  MemoryLifetimeVerifier verifier(this);
+void SILFunction::verifyMemoryLifetime(SILPassManager *passManager) {
+  MemoryLifetimeVerifier verifier(this, passManager);
   verifier.verify();
 }

lib/SIL/Verifier/SILVerifier.cpp

@@ -724,6 +724,7 @@ static void checkAddressWalkerCanVisitAllTransitiveUses(SILValue address) {
 class SILVerifier : public SILVerifierBase<SILVerifier> {
   ModuleDecl *M;
   const SILFunction &F;
+  SILPassManager *passManager;
   SILFunctionConventions fnConv;
   Lowering::TypeConverter &TC;
 
@@ -993,8 +994,10 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     return numInsts;
   }
 
-  SILVerifier(const SILFunction &F, bool SingleFunction, bool checkLinearLifetime)
+  SILVerifier(const SILFunction &F, SILPassManager *passManager,
+              bool SingleFunction, bool checkLinearLifetime)
       : M(F.getModule().getSwiftModule()), F(F),
+        passManager(passManager),
         fnConv(F.getConventionsInContext()), TC(F.getModule().Types),
         SingleFunction(SingleFunction),
         checkLinearLifetime(checkLinearLifetime),
@@ -6532,7 +6535,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
 
     if (F->hasOwnership() && F->shouldVerifyOwnership() &&
         !mod.getASTContext().hadError()) {
-      F->verifyMemoryLifetime();
+      F->verifyMemoryLifetime(passManager);
     }
   }
 
@@ -6572,22 +6575,25 @@ static bool verificationEnabled(const SILModule &M) {
 
 /// verify - Run the SIL verifier to make sure that the SILFunction follows
 /// invariants.
-void SILFunction::verify(bool SingleFunction, bool isCompleteOSSA,
+void SILFunction::verify(SILPassManager *passManager,
+                         bool SingleFunction, bool isCompleteOSSA,
                          bool checkLinearLifetime) const {
   if (!verificationEnabled(getModule()))
     return;
 
   // Please put all checks in visitSILFunction in SILVerifier, not here. This
   // ensures that the pretty stack trace in the verifier is included with the
   // back trace when the verifier crashes.
-  SILVerifier(*this, SingleFunction, checkLinearLifetime).verify(isCompleteOSSA);
+  SILVerifier verifier(*this, passManager, SingleFunction, checkLinearLifetime);
+  verifier.verify(isCompleteOSSA);
 }
 
 void SILFunction::verifyCriticalEdges() const {
   if (!verificationEnabled(getModule()))
     return;
 
-  SILVerifier(*this, /*SingleFunction=*/true,
+  SILVerifier(*this, /*passManager=*/nullptr,
+                     /*SingleFunction=*/true,
                      /*checkLinearLifetime=*/ false).verifyBranches(this);
 }
 
@@ -6705,7 +6711,8 @@ void SILVTable::verify(const SILModule &M) const {
     }
 
     if (M.getStage() != SILStage::Lowered) {
-      SILVerifier(*entry.getImplementation(), /*SingleFunction=*/true,
+      SILVerifier(*entry.getImplementation(), /*passManager=*/nullptr,
+                                              /*SingleFunction=*/true,
                                               /*checkLinearLifetime=*/ false)
           .requireABICompatibleFunctionTypes(
               baseInfo.getSILType().castTo<SILFunctionType>(),
@@ -6850,8 +6857,14 @@ void SILGlobalVariable::verify() const {
   }
 }
 
-/// Verify the module.
 void SILModule::verify(bool isCompleteOSSA, bool checkLinearLifetime) const {
+  SILPassManager pm(const_cast<SILModule *>(this), /*isMandatory=*/false, /*IRMod=*/nullptr);
+  verify(&pm, isCompleteOSSA, checkLinearLifetime);
+}
+
+/// Verify the module.
+void SILModule::verify(SILPassManager *passManager,
+                       bool isCompleteOSSA, bool checkLinearLifetime) const {
   if (!verificationEnabled(*this))
     return;
 
@@ -6866,7 +6879,7 @@ void SILModule::verify(bool isCompleteOSSA, bool checkLinearLifetime) const {
       llvm::errs() << "Symbol redefined: " << f.getName() << "!\n";
       assert(false && "triggering standard assertion failure routine");
     }
-    f.verify(/*singleFunction*/ false, isCompleteOSSA, checkLinearLifetime);
+    f.verify(passManager, /*singleFunction*/ false, isCompleteOSSA, checkLinearLifetime);
   }
 
   // Check all globals.

lib/SILOptimizer/Analysis/BasicCalleeAnalysis.cpp

@@ -352,9 +352,9 @@ void BridgedCalleeAnalysis::registerAnalysis(IsDeinitBarrierFn instructionIsDein
 MemoryBehavior BasicCalleeAnalysis::
 getMemoryBehavior(ApplySite as, bool observeRetains) {
   if (getMemBehvaiorFunction) {
-    auto b = getMemBehvaiorFunction({pm->getSwiftPassInvocation()},
-                                    {as.getInstruction()->asSILNode()},
-                                    observeRetains);
+    auto b = getMemBehvaiorFunction({as.getInstruction()->asSILNode()},
+                                    observeRetains,
+                                    {this});
     return (MemoryBehavior)b;
   }
   return MemoryBehavior::MayHaveSideEffects;

lib/SILOptimizer/LoopTransforms/ArrayBoundsCheckOpts.cpp

@@ -1271,7 +1271,7 @@ bool ABCOpt::processLoop(SILLoop *Loop) {
   Changed |= hoistChecksInLoop(DT->getNode(Header), ABC, IndVars, Preheader,
                                Header, SingleExitingBlk, /*recursionDepth*/ 0);
   if (Changed) {
-    Preheader->getParent()->verify();
+    Preheader->getParent()->verify(getPassManager());
   }
   return Changed;
 }

lib/SILOptimizer/Mandatory/ClosureLifetimeFixup.cpp

@@ -1251,7 +1251,7 @@ class ClosureLifetimeFixup : public SILFunctionTransform {
       }
       invalidateAnalysis(analysisInvalidationKind(modifiedCFG));
     }
-    LLVM_DEBUG(getFunction()->verify());
+    LLVM_DEBUG(getFunction()->verify(getPassManager()));
 
   }
 

lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp

@@ -751,7 +751,7 @@ struct OwnershipModelEliminator : SILFunctionTransform {
           "Found verification error when verifying before lowering "
           "ownership. Please re-run with -sil-verify-all to identify the "
           "actual pass that introduced the verification error.");
-      f->verify();
+      f->verify(getPassManager());
     }
 
     if (stripOwnership(*f)) {