From a1d95c3fc426f1fc54758866d8a0e8718a82a745 Mon Sep 17 00:00:00 2001 From: Fedor Sergeev Date: Tue, 11 Dec 2018 19:05:35 +0000 Subject: [PATCH] [NewPM] fixing asserts on deleted loop in -print-after-all IR-printing AfterPass instrumentation might be called on a loop that has just been invalidated. We should skip printing it to avoid spurious asserts. Reviewed By: chandlerc, philip.pfaffe Differential Revision: https://reviews.llvm.org/D54740 llvm-svn: 348887 --- llvm/include/llvm/Analysis/CGSCCPassManager.h | 10 +- llvm/include/llvm/IR/PassInstrumentation.h | 31 +++++- llvm/include/llvm/IR/PassTimingInfo.h | 4 +- .../llvm/Transforms/Scalar/LoopPassManager.h | 6 +- llvm/lib/Analysis/CGSCCPassManager.cpp | 5 +- llvm/lib/IR/PassTimingInfo.cpp | 14 ++- llvm/lib/Passes/StandardInstrumentations.cpp | 1 - llvm/lib/Transforms/Scalar/LoopPassManager.cpp | 6 +- llvm/test/Other/loop-deletion-printer.ll | 24 ++++ llvm/test/Other/scc-deleted-printer.ll | 25 +++++ llvm/unittests/IR/PassBuilderCallbacksTest.cpp | 124 +++++++++++++++++++++ 11 files changed, 233 insertions(+), 17 deletions(-) create mode 100644 llvm/test/Other/loop-deletion-printer.ll create mode 100644 llvm/test/Other/scc-deleted-printer.ll diff --git a/llvm/include/llvm/Analysis/CGSCCPassManager.h b/llvm/include/llvm/Analysis/CGSCCPassManager.h index f150064..61b99f6 100644 --- a/llvm/include/llvm/Analysis/CGSCCPassManager.h +++ b/llvm/include/llvm/Analysis/CGSCCPassManager.h @@ -441,7 +441,10 @@ public: PreservedAnalyses PassPA = Pass.run(*C, CGAM, CG, UR); - PI.runAfterPass(Pass, *C); + if (UR.InvalidatedSCCs.count(C)) + PI.runAfterPassInvalidated(Pass); + else + PI.runAfterPass(Pass, *C); // Update the SCC and RefSCC if necessary. C = UR.UpdatedC ? UR.UpdatedC : C; @@ -762,7 +765,10 @@ public: PreservedAnalyses PassPA = Pass.run(*C, AM, CG, UR); - PI.runAfterPass(Pass, *C); + if (UR.InvalidatedSCCs.count(C)) + PI.runAfterPassInvalidated(Pass); + else + PI.runAfterPass(Pass, *C); // If the SCC structure has changed, bail immediately and let the outer // CGSCC layer handle any iteration to reflect the refined structure. diff --git a/llvm/include/llvm/IR/PassInstrumentation.h b/llvm/include/llvm/IR/PassInstrumentation.h index 3718b53..08dac1c 100644 --- a/llvm/include/llvm/IR/PassInstrumentation.h +++ b/llvm/include/llvm/IR/PassInstrumentation.h @@ -68,10 +68,17 @@ class PreservedAnalyses; /// PassInstrumentation to pass control to the registered callbacks. class PassInstrumentationCallbacks { public: - // Before/After callbacks accept IRUnits, so they need to take them - // as pointers, wrapped with llvm::Any + // Before/After callbacks accept IRUnits whenever appropriate, so they need + // to take them as constant pointers, wrapped with llvm::Any. + // For the case when IRUnit has been invalidated there is a different + // callback to use - AfterPassInvalidated. + // TODO: currently AfterPassInvalidated does not accept IRUnit, since passing + // already invalidated IRUnit is unsafe. There are ways to handle invalidated IRUnits + // in a safe way, and we might pursue that as soon as there is a useful instrumentation + // that needs it. using BeforePassFunc = bool(StringRef, Any); using AfterPassFunc = void(StringRef, Any); + using AfterPassInvalidatedFunc = void(StringRef); using BeforeAnalysisFunc = void(StringRef, Any); using AfterAnalysisFunc = void(StringRef, Any); @@ -91,6 +98,11 @@ public: } template + void registerAfterPassInvalidatedCallback(CallableT C) { + AfterPassInvalidatedCallbacks.emplace_back(std::move(C)); + } + + template void registerBeforeAnalysisCallback(CallableT C) { BeforeAnalysisCallbacks.emplace_back(std::move(C)); } @@ -105,6 +117,8 @@ private: SmallVector, 4> BeforePassCallbacks; SmallVector, 4> AfterPassCallbacks; + SmallVector, 4> + AfterPassInvalidatedCallbacks; SmallVector, 4> BeforeAnalysisCallbacks; SmallVector, 4> @@ -139,7 +153,8 @@ public: } /// AfterPass instrumentation point - takes \p Pass instance that has - /// just been executed and constant reference to IR it operates on. + /// just been executed and constant reference to \p IR it operates on. + /// \p IR is guaranteed to be valid at this point. template void runAfterPass(const PassT &Pass, const IRUnitT &IR) const { if (Callbacks) @@ -147,6 +162,16 @@ public: C(Pass.name(), llvm::Any(&IR)); } + /// AfterPassInvalidated instrumentation point - takes \p Pass instance + /// that has just been executed. For use when IR has been invalidated + /// by \p Pass execution. + template + void runAfterPassInvalidated(const PassT &Pass) const { + if (Callbacks) + for (auto &C : Callbacks->AfterPassInvalidatedCallbacks) + C(Pass.name()); + } + /// BeforeAnalysis instrumentation point - takes \p Analysis instance /// to be executed and constant reference to IR it operates on. template diff --git a/llvm/include/llvm/IR/PassTimingInfo.h b/llvm/include/llvm/IR/PassTimingInfo.h index 2c6a0e0..e9945f9 100644 --- a/llvm/include/llvm/IR/PassTimingInfo.h +++ b/llvm/include/llvm/IR/PassTimingInfo.h @@ -99,8 +99,8 @@ private: void stopTimer(StringRef PassID); // Implementation of pass instrumentation callbacks. - bool runBeforePass(StringRef PassID, Any IR); - void runAfterPass(StringRef PassID, Any IR); + bool runBeforePass(StringRef PassID); + void runAfterPass(StringRef PassID); }; } // namespace llvm diff --git a/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h index e54960d..46ebb74 100644 --- a/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h +++ b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h @@ -352,7 +352,11 @@ public: continue; PreservedAnalyses PassPA = Pass.run(*L, LAM, LAR, Updater); - PI.runAfterPass(Pass, *L); + // Do not pass deleted Loop into the instrumentation. + if (Updater.skipCurrentLoop()) + PI.runAfterPassInvalidated(Pass); + else + PI.runAfterPass(Pass, *L); // FIXME: We should verify the set of analyses relevant to Loop passes // are preserved. diff --git a/llvm/lib/Analysis/CGSCCPassManager.cpp b/llvm/lib/Analysis/CGSCCPassManager.cpp index 6965235..fd2292c 100644 --- a/llvm/lib/Analysis/CGSCCPassManager.cpp +++ b/llvm/lib/Analysis/CGSCCPassManager.cpp @@ -79,7 +79,10 @@ PassManagerrun(*C, AM, G, UR); - PI.runAfterPass(*Pass, *C); + if (UR.InvalidatedSCCs.count(C)) + PI.runAfterPassInvalidated(*Pass); + else + PI.runAfterPass(*Pass, *C); // Update the SCC if necessary. C = UR.UpdatedC ? UR.UpdatedC : C; diff --git a/llvm/lib/IR/PassTimingInfo.cpp b/llvm/lib/IR/PassTimingInfo.cpp index 8165704..40b3977 100644 --- a/llvm/lib/IR/PassTimingInfo.cpp +++ b/llvm/lib/IR/PassTimingInfo.cpp @@ -226,7 +226,7 @@ static bool matchPassManager(StringRef PassID) { Prefix.endswith("AnalysisManagerProxy"); } -bool TimePassesHandler::runBeforePass(StringRef PassID, Any IR) { +bool TimePassesHandler::runBeforePass(StringRef PassID) { if (matchPassManager(PassID)) return true; @@ -239,7 +239,7 @@ bool TimePassesHandler::runBeforePass(StringRef PassID, Any IR) { return true; } -void TimePassesHandler::runAfterPass(StringRef PassID, Any IR) { +void TimePassesHandler::runAfterPass(StringRef PassID) { if (matchPassManager(PassID)) return; @@ -254,13 +254,15 @@ void TimePassesHandler::registerCallbacks(PassInstrumentationCallbacks &PIC) { return; PIC.registerBeforePassCallback( - [this](StringRef P, Any IR) { return this->runBeforePass(P, IR); }); + [this](StringRef P, Any) { return this->runBeforePass(P); }); PIC.registerAfterPassCallback( - [this](StringRef P, Any IR) { this->runAfterPass(P, IR); }); + [this](StringRef P, Any) { this->runAfterPass(P); }); + PIC.registerAfterPassInvalidatedCallback( + [this](StringRef P) { this->runAfterPass(P); }); PIC.registerBeforeAnalysisCallback( - [this](StringRef P, Any IR) { this->runBeforePass(P, IR); }); + [this](StringRef P, Any) { this->runBeforePass(P); }); PIC.registerAfterAnalysisCallback( - [this](StringRef P, Any IR) { this->runAfterPass(P, IR); }); + [this](StringRef P, Any) { this->runAfterPass(P); }); } } // namespace llvm diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp index 48d36e5..765ffe6 100644 --- a/llvm/lib/Passes/StandardInstrumentations.cpp +++ b/llvm/lib/Passes/StandardInstrumentations.cpp @@ -53,7 +53,6 @@ void unwrapAndPrint(StringRef Banner, Any IR) { Extra = formatv(" (function: {0})\n", F->getName()); } else if (any_isa(IR)) { const LazyCallGraph::SCC *C = any_cast(IR); - assert(C); if (!llvm::forcePrintModuleIR()) { Extra = formatv(" (scc: {0})\n", C->getName()); bool BannerPrinted = false; diff --git a/llvm/lib/Transforms/Scalar/LoopPassManager.cpp b/llvm/lib/Transforms/Scalar/LoopPassManager.cpp index 6759bd2..774ad7b 100644 --- a/llvm/lib/Transforms/Scalar/LoopPassManager.cpp +++ b/llvm/lib/Transforms/Scalar/LoopPassManager.cpp @@ -44,7 +44,11 @@ PassManagerrun(L, AM, AR, U); - PI.runAfterPass(*Pass, L); + // do not pass deleted Loop into the instrumentation + if (U.skipCurrentLoop()) + PI.runAfterPassInvalidated(*Pass); + else + PI.runAfterPass(*Pass, L); // If the loop was deleted, abort the run and return to the outer walk. if (U.skipCurrentLoop()) { diff --git a/llvm/test/Other/loop-deletion-printer.ll b/llvm/test/Other/loop-deletion-printer.ll new file mode 100644 index 0000000..d344568 --- /dev/null +++ b/llvm/test/Other/loop-deletion-printer.ll @@ -0,0 +1,24 @@ +; Make sure that Loop which was invalidated by loop-deletion +; does not lead to problems for -print-after-all and is just skipped. +; +; RUN: opt < %s -disable-output \ +; RUN: -passes=loop-instsimplify -print-after-all 2>&1 | FileCheck %s -check-prefix=SIMPLIFY +; RUN: opt < %s -disable-output \ +; RUN: -passes=loop-deletion,loop-instsimplify -print-after-all 2>&1 | FileCheck %s -check-prefix=DELETED +; +; SIMPLIFY: IR Dump {{.*}} LoopInstSimplifyPass +; DELETED-NOT: IR Dump {{.*}}LoopInstSimplifyPass +; DELETED-NOT: IR Dump {{.*}}LoopDeletionPass + +define void @deleteme() { +entry: + br label %loop +loop: + %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ] + %iv.next = add i32 %iv, 1 + %check = icmp ult i32 %iv.next, 3 + br i1 %check, label %loop, label %exit +exit: + ret void +} + diff --git a/llvm/test/Other/scc-deleted-printer.ll b/llvm/test/Other/scc-deleted-printer.ll new file mode 100644 index 0000000..e6323f0 --- /dev/null +++ b/llvm/test/Other/scc-deleted-printer.ll @@ -0,0 +1,25 @@ +; RUN: opt < %s 2>&1 -disable-output \ +; RUN: -passes=inline -print-before-all -print-after-all | FileCheck %s -check-prefix=INL +; RUN: opt < %s 2>&1 -disable-output \ +; RUN: -passes=inline -print-before-all -print-after-all -print-module-scope | FileCheck %s -check-prefix=INL-MOD + +; INL: IR Dump Before {{InlinerPass .*scc: .tester, foo}} +; INL-NOT: IR Dump After {{InlinerPass}} +; INL: IR Dump Before {{InlinerPass .*scc: .tester}} +; INL: IR Dump After {{InlinerPass .*scc: .tester}} + +; INL-MOD: IR Dump Before {{InlinerPass .*scc: .tester, foo}} +; INL-MOD-NOT: IR Dump After {{InlinerPass}} +; INL-MOD: IR Dump Before {{InlinerPass .*scc: .tester}} +; INL-MOD: IR Dump After {{InlinerPass .*scc: .tester}} + + +define void @tester() noinline { + call void @foo() + ret void +} + +define internal void @foo() alwaysinline { + call void @tester() + ret void +} diff --git a/llvm/unittests/IR/PassBuilderCallbacksTest.cpp b/llvm/unittests/IR/PassBuilderCallbacksTest.cpp index 20c47b0..63fdc67 100644 --- a/llvm/unittests/IR/PassBuilderCallbacksTest.cpp +++ b/llvm/unittests/IR/PassBuilderCallbacksTest.cpp @@ -167,6 +167,11 @@ struct MockPassHandle MOCK_METHOD4(run, PreservedAnalyses(Loop &, LoopAnalysisManager &, LoopStandardAnalysisResults &, LPMUpdater &)); + static void invalidateLoop(Loop &L, LoopAnalysisManager &, + LoopStandardAnalysisResults &, + LPMUpdater &Updater) { + Updater.markLoopAsDeleted(L, L.getName()); + } MockPassHandle() { setDefaults(); } }; @@ -187,6 +192,11 @@ struct MockPassHandle PreservedAnalyses(LazyCallGraph::SCC &, CGSCCAnalysisManager &, LazyCallGraph &G, CGSCCUpdateResult &UR)); + static void invalidateSCC(LazyCallGraph::SCC &C, CGSCCAnalysisManager &, + LazyCallGraph &, CGSCCUpdateResult &UR) { + UR.InvalidatedSCCs.insert(&C); + } + MockPassHandle() { setDefaults(); } }; @@ -310,6 +320,7 @@ struct MockPassInstrumentationCallbacks { } MOCK_METHOD2(runBeforePass, bool(StringRef PassID, llvm::Any)); MOCK_METHOD2(runAfterPass, void(StringRef PassID, llvm::Any)); + MOCK_METHOD1(runAfterPassInvalidated, void(StringRef PassID)); MOCK_METHOD2(runBeforeAnalysis, void(StringRef PassID, llvm::Any)); MOCK_METHOD2(runAfterAnalysis, void(StringRef PassID, llvm::Any)); @@ -319,6 +330,8 @@ struct MockPassInstrumentationCallbacks { }); Callbacks.registerAfterPassCallback( [this](StringRef P, llvm::Any IR) { this->runAfterPass(P, IR); }); + Callbacks.registerAfterPassInvalidatedCallback( + [this](StringRef P) { this->runAfterPassInvalidated(P); }); Callbacks.registerBeforeAnalysisCallback([this](StringRef P, llvm::Any IR) { return this->runBeforeAnalysis(P, IR); }); @@ -571,6 +584,11 @@ TEST_F(FunctionCallbacksTest, InstrumentedPasses) { runAfterPass(HasNameRegex("MockPassHandle"), HasName("foo"))) .InSequence(PISequence); + // Our mock pass does not invalidate IR. + EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("MockPassHandle"))) + .Times(0); + StringRef PipelineText = "test-transform"; ASSERT_THAT_ERROR(PB.parsePassPipeline(PM, PipelineText, true), Succeeded()) << "Pipeline was: " << PipelineText; @@ -595,6 +613,9 @@ TEST_F(FunctionCallbacksTest, InstrumentedSkippedPasses) { // as well. EXPECT_CALL(CallbacksHandle, runAfterPass(HasNameRegex("MockPassHandle"), _)) .Times(0); + EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("MockPassHandle"))) + .Times(0); EXPECT_CALL(CallbacksHandle, runAfterPass(HasNameRegex("MockPassHandle"), _)) .Times(0); EXPECT_CALL(CallbacksHandle, @@ -650,6 +671,55 @@ TEST_F(LoopCallbacksTest, InstrumentedPasses) { runAfterPass(HasNameRegex("MockPassHandle"), HasName("loop"))) .InSequence(PISequence); + // Our mock pass does not invalidate IR. + EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("MockPassHandle"))) + .Times(0); + + StringRef PipelineText = "test-transform"; + ASSERT_THAT_ERROR(PB.parsePassPipeline(PM, PipelineText, true), Succeeded()) + << "Pipeline was: " << PipelineText; + PM.run(*M, AM); +} + +TEST_F(LoopCallbacksTest, InstrumentedInvalidatingPasses) { + CallbacksHandle.registerPassInstrumentation(); + // Non-mock instrumentation not specifically mentioned below can be ignored. + CallbacksHandle.ignoreNonMockPassInstrumentation(""); + CallbacksHandle.ignoreNonMockPassInstrumentation("foo"); + CallbacksHandle.ignoreNonMockPassInstrumentation("loop"); + + EXPECT_CALL(AnalysisHandle, run(HasName("loop"), _, _)); + EXPECT_CALL(PassHandle, run(HasName("loop"), _, _, _)) + .WillOnce(DoAll(WithArgs<0, 1, 2, 3>(Invoke(PassHandle.invalidateLoop)), + WithArgs<0, 1, 2>(Invoke(getAnalysisResult)))); + + // PassInstrumentation calls should happen in-sequence, in the same order + // as passes/analyses are scheduled. + ::testing::Sequence PISequence; + EXPECT_CALL(CallbacksHandle, + runBeforePass(HasNameRegex("MockPassHandle"), HasName("loop"))) + .InSequence(PISequence); + EXPECT_CALL( + CallbacksHandle, + runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("loop"))) + .InSequence(PISequence); + EXPECT_CALL( + CallbacksHandle, + runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("loop"))) + .InSequence(PISequence); + EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("MockPassHandle"))) + .InSequence(PISequence); + EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("^PassManager"))) + .InSequence(PISequence); + + // Our mock pass invalidates IR, thus normal runAfterPass is never called. + EXPECT_CALL(CallbacksHandle, + runAfterPass(HasNameRegex("MockPassHandle"), HasName("loop"))) + .Times(0); + StringRef PipelineText = "test-transform"; ASSERT_THAT_ERROR(PB.parsePassPipeline(PM, PipelineText, true), Succeeded()) << "Pipeline was: " << PipelineText; @@ -676,6 +746,9 @@ TEST_F(LoopCallbacksTest, InstrumentedSkippedPasses) { EXPECT_CALL(CallbacksHandle, runAfterPass(HasNameRegex("MockPassHandle"), _)) .Times(0); EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("MockPassHandle"))) + .Times(0); + EXPECT_CALL(CallbacksHandle, runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), _)) .Times(0); EXPECT_CALL(CallbacksHandle, @@ -727,6 +800,54 @@ TEST_F(CGSCCCallbacksTest, InstrumentedPasses) { runAfterPass(HasNameRegex("MockPassHandle"), HasName("(foo)"))) .InSequence(PISequence); + // Our mock pass does not invalidate IR. + EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("MockPassHandle"))) + .Times(0); + + StringRef PipelineText = "test-transform"; + ASSERT_THAT_ERROR(PB.parsePassPipeline(PM, PipelineText, true), Succeeded()) + << "Pipeline was: " << PipelineText; + PM.run(*M, AM); +} + +TEST_F(CGSCCCallbacksTest, InstrumentedInvalidatingPasses) { + CallbacksHandle.registerPassInstrumentation(); + // Non-mock instrumentation not specifically mentioned below can be ignored. + CallbacksHandle.ignoreNonMockPassInstrumentation(""); + CallbacksHandle.ignoreNonMockPassInstrumentation("(foo)"); + + EXPECT_CALL(AnalysisHandle, run(HasName("(foo)"), _, _)); + EXPECT_CALL(PassHandle, run(HasName("(foo)"), _, _, _)) + .WillOnce(DoAll(WithArgs<0, 1, 2, 3>(Invoke(PassHandle.invalidateSCC)), + WithArgs<0, 1, 2>(Invoke(getAnalysisResult)))); + + // PassInstrumentation calls should happen in-sequence, in the same order + // as passes/analyses are scheduled. + ::testing::Sequence PISequence; + EXPECT_CALL(CallbacksHandle, + runBeforePass(HasNameRegex("MockPassHandle"), HasName("(foo)"))) + .InSequence(PISequence); + EXPECT_CALL( + CallbacksHandle, + runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("(foo)"))) + .InSequence(PISequence); + EXPECT_CALL( + CallbacksHandle, + runAfterAnalysis(HasNameRegex("MockAnalysisHandle"), HasName("(foo)"))) + .InSequence(PISequence); + EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("MockPassHandle"))) + .InSequence(PISequence); + EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("^PassManager"))) + .InSequence(PISequence); + + // Our mock pass does invalidate IR, thus normal runAfterPass is never called. + EXPECT_CALL(CallbacksHandle, + runAfterPass(HasNameRegex("MockPassHandle"), HasName("(foo)"))) + .Times(0); + StringRef PipelineText = "test-transform"; ASSERT_THAT_ERROR(PB.parsePassPipeline(PM, PipelineText, true), Succeeded()) << "Pipeline was: " << PipelineText; @@ -753,6 +874,9 @@ TEST_F(CGSCCCallbacksTest, InstrumentedSkippedPasses) { EXPECT_CALL(CallbacksHandle, runAfterPass(HasNameRegex("MockPassHandle"), _)) .Times(0); EXPECT_CALL(CallbacksHandle, + runAfterPassInvalidated(HasNameRegex("MockPassHandle"))) + .Times(0); + EXPECT_CALL(CallbacksHandle, runBeforeAnalysis(HasNameRegex("MockAnalysisHandle"), _)) .Times(0); EXPECT_CALL(CallbacksHandle, -- 2.7.4