[llvm] r307498 - [PM] Fix a nasty bug in the new PM where we failed to properly
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 10 10:27:51 PDT 2017
On Sun, Jul 9, 2017 at 6:45 AM Chandler Carruth via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: chandlerc
> Date: Sun Jul 9 06:45:11 2017
> New Revision: 307498
>
> URL: http://llvm.org/viewvc/llvm-project?rev=307498&view=rev
> Log:
> [PM] Fix a nasty bug in the new PM where we failed to properly
> invalidation of analyses when merging SCCs.
>
> While I've added a bunch of testing of this, it takes something much
> more like the inliner to really trigger this as you need to have
> partially-analyzed SCCs with updates at just the right time.
Why is that ^ not reproducible via a unit test with a dummy/stub
analysis/transform/etc? Too much boilerplate to write those?
> So I've
> added a direct test for this using the inliner and verifying the
> domtree. Without the changes here, this test ends up finding a stale
> dominator tree.
>
> However, to handle this properly, we need to invalidate analyses
> *before* merging the SCCs. After talking to Philip and Sanjoy about this
> they convinced me this was the right approach. To do this, we need
> a callback mechanism when merging SCCs so we can observe the cycle that
> will be merged before the merge happens. This API update ended up being
> surprisingly easy.
>
> With this commit, the new PM passes the test-suite again. It hadn't
> since MemorySSA was enabled for EarlyCSE as that also will find this bug
> very quickly.
>
> Modified:
> llvm/trunk/include/llvm/Analysis/LazyCallGraph.h
> llvm/trunk/lib/Analysis/CGSCCPassManager.cpp
> llvm/trunk/lib/Analysis/LazyCallGraph.cpp
> llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll
> llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp
>
> Modified: llvm/trunk/include/llvm/Analysis/LazyCallGraph.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/LazyCallGraph.h?rev=307498&r1=307497&r2=307498&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/LazyCallGraph.h (original)
> +++ llvm/trunk/include/llvm/Analysis/LazyCallGraph.h Sun Jul 9 06:45:11
> 2017
> @@ -652,17 +652,23 @@ public:
> /// Make an existing internal ref edge into a call edge.
> ///
> /// This may form a larger cycle and thus collapse SCCs into
> TargetN's SCC.
> - /// If that happens, the deleted SCC pointers are returned. These
> SCCs are
> - /// not in a valid state any longer but the pointers will remain valid
> - /// until destruction of the parent graph instance for the purpose of
> - /// clearing cached information.
> + /// If that happens, the optional callback \p MergedCB will be
> invoked (if
> + /// provided) on the SCCs being merged away prior to actually
> performing
> + /// the merge. Note that this will never include the target SCC as
> that
> + /// will be the SCC functions are merged into to resolve the cycle.
> Once
> + /// this function returns, these merged SCCs are not in a valid state
> but
> + /// the pointers will remain valid until destruction of the parent
> graph
> + /// instance for the purpose of clearing cached information. This
> function
> + /// also returns 'true' if a cycle was formed and some SCCs merged
> away as
> + /// a convenience.
> ///
> /// After this operation, both SourceN's SCC and TargetN's SCC may
> move
> /// position within this RefSCC's postorder list. Any SCCs merged are
> /// merged into the TargetN's SCC in order to preserve reachability
> analyses
> /// which took place on that SCC.
> - SmallVector<SCC *, 1> switchInternalEdgeToCall(Node &SourceN,
> - Node &TargetN);
> + bool switchInternalEdgeToCall(
> + Node &SourceN, Node &TargetN,
> + function_ref<void(ArrayRef<SCC *> MergedSCCs)> MergeCB = {});
>
> /// Make an existing internal call edge between separate SCCs into a
> ref
> /// edge.
>
> Modified: llvm/trunk/lib/Analysis/CGSCCPassManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/CGSCCPassManager.cpp?rev=307498&r1=307497&r2=307498&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Analysis/CGSCCPassManager.cpp (original)
> +++ llvm/trunk/lib/Analysis/CGSCCPassManager.cpp Sun Jul 9 06:45:11 2017
> @@ -570,25 +570,48 @@ LazyCallGraph::SCC &llvm::updateCGAndAna
> // Otherwise we are switching an internal ref edge to a call edge.
> This
> // may merge away some SCCs, and we add those to the UpdateResult. We
> also
> // need to make sure to update the worklist in the event SCCs have
> moved
> - // before the current one in the post-order sequence.
> + // before the current one in the post-order sequence
> + bool HasFunctionAnalysisProxy = false;
> auto InitialSCCIndex = RC->find(*C) - RC->begin();
> - auto InvalidatedSCCs = RC->switchInternalEdgeToCall(N, *CallTarget);
> - if (!InvalidatedSCCs.empty()) {
> + bool FormedCycle = RC->switchInternalEdgeToCall(
> + N, *CallTarget, [&](ArrayRef<SCC *> MergedSCCs) {
> + for (SCC *MergedC : MergedSCCs) {
> + assert(MergedC != &TargetC && "Cannot merge away the target
> SCC!");
> +
> + HasFunctionAnalysisProxy |=
> + AM.getCachedResult<FunctionAnalysisManagerCGSCCProxy>(
> + *MergedC) != nullptr;
> +
> + // Mark that this SCC will no longer be valid.
> + UR.InvalidatedSCCs.insert(MergedC);
> +
> + // FIXME: We should really do a 'clear' here to forcibly
> release
> + // memory, but we don't have a good way of doing that and
> + // preserving the function analyses.
> + auto PA =
> PreservedAnalyses::allInSet<AllAnalysesOn<Function>>();
> + PA.preserve<FunctionAnalysisManagerCGSCCProxy>();
> + AM.invalidate(*MergedC, PA);
> + }
> + });
> +
> + // If we formed a cycle by creating this call, we need to update more
> data
> + // structures.
> + if (FormedCycle) {
> C = &TargetC;
> assert(G.lookupSCC(N) == C && "Failed to update current SCC!");
>
> - // Any analyses cached for this SCC are no longer precise as the
> shape
> - // has changed by introducing this cycle.
> - AM.invalidate(*C, PreservedAnalyses::none());
> + // If one of the invalidated SCCs had a cached proxy to a function
> + // analysis manager, we need to create a proxy in the new current
> SCC as
> + // the invaliadted SCCs had their functions moved.
> + if (HasFunctionAnalysisProxy)
> + AM.getResult<FunctionAnalysisManagerCGSCCProxy>(*C, G);
>
> - for (SCC *InvalidatedC : InvalidatedSCCs) {
> - assert(InvalidatedC != C && "Cannot invalidate the current SCC!");
> - UR.InvalidatedSCCs.insert(InvalidatedC);
> -
> - // Also clear any cached analyses for the SCCs that are dead. This
> - // isn't really necessary for correctness but can release memory.
> - AM.clear(*InvalidatedC);
> - }
> + // Any analyses cached for this SCC are no longer precise as the
> shape
> + // has changed by introducing this cycle. However, we have taken
> care to
> + // update the proxies so it remains valide.
> + auto PA = PreservedAnalyses::allInSet<AllAnalysesOn<Function>>();
> + PA.preserve<FunctionAnalysisManagerCGSCCProxy>();
> + AM.invalidate(*C, PA);
> }
> auto NewSCCIndex = RC->find(*C) - RC->begin();
> if (InitialSCCIndex < NewSCCIndex) {
>
> Modified: llvm/trunk/lib/Analysis/LazyCallGraph.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LazyCallGraph.cpp?rev=307498&r1=307497&r2=307498&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Analysis/LazyCallGraph.cpp (original)
> +++ llvm/trunk/lib/Analysis/LazyCallGraph.cpp Sun Jul 9 06:45:11 2017
> @@ -456,8 +456,10 @@ updatePostorderSequenceForEdgeInsertion(
> return make_range(SCCs.begin() + SourceIdx, SCCs.begin() + TargetIdx);
> }
>
> -SmallVector<LazyCallGraph::SCC *, 1>
> -LazyCallGraph::RefSCC::switchInternalEdgeToCall(Node &SourceN, Node
> &TargetN) {
> +bool
> +LazyCallGraph::RefSCC::switchInternalEdgeToCall(
> + Node &SourceN, Node &TargetN,
> + function_ref<void(ArrayRef<SCC *> MergeSCCs)> MergeCB) {
> assert(!(*SourceN)[TargetN].isCall() && "Must start with a ref edge!");
> SmallVector<SCC *, 1> DeletedSCCs;
>
> @@ -475,7 +477,7 @@ LazyCallGraph::RefSCC::switchInternalEdg
> // we've just added more connectivity.
> if (&SourceSCC == &TargetSCC) {
> SourceN->setEdgeKind(TargetN, Edge::Call);
> - return DeletedSCCs;
> + return false; // No new cycle.
> }
>
> // At this point we leverage the postorder list of SCCs to detect when
> the
> @@ -488,7 +490,7 @@ LazyCallGraph::RefSCC::switchInternalEdg
> int TargetIdx = SCCIndices[&TargetSCC];
> if (TargetIdx < SourceIdx) {
> SourceN->setEdgeKind(TargetN, Edge::Call);
> - return DeletedSCCs;
> + return false; // No new cycle.
> }
>
> // Compute the SCCs which (transitively) reach the source.
> @@ -555,12 +557,16 @@ LazyCallGraph::RefSCC::switchInternalEdg
> SourceSCC, TargetSCC, SCCs, SCCIndices, ComputeSourceConnectedSet,
> ComputeTargetConnectedSet);
>
> + // Run the user's callback on the merged SCCs before we actually merge
> them.
> + if (MergeCB)
> + MergeCB(makeArrayRef(MergeRange.begin(), MergeRange.end()));
> +
> // If the merge range is empty, then adding the edge didn't actually
> form any
> // new cycles. We're done.
> if (MergeRange.begin() == MergeRange.end()) {
> // Now that the SCC structure is finalized, flip the kind to call.
> SourceN->setEdgeKind(TargetN, Edge::Call);
> - return DeletedSCCs;
> + return false; // No new cycle.
> }
>
> #ifndef NDEBUG
> @@ -596,8 +602,8 @@ LazyCallGraph::RefSCC::switchInternalEdg
> // Now that the SCC structure is finalized, flip the kind to call.
> SourceN->setEdgeKind(TargetN, Edge::Call);
>
> - // And we're done!
> - return DeletedSCCs;
> + // And we're done, but we did form a new cycle.
> + return true;
> }
>
> void LazyCallGraph::RefSCC::switchTrivialInternalEdgeToRef(Node &SourceN,
>
> Modified: llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll?rev=307498&r1=307497&r2=307498&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll
> (original)
> +++ llvm/trunk/test/Transforms/Inline/cgscc-incremental-invalidate.ll Sun
> Jul 9 06:45:11 2017
> @@ -127,3 +127,80 @@ entry:
> ret void
> ; CHECK: ret void
> }
> +
> +; The 'test2_' prefixed code works to carefully trigger forming an SCC
> with
> +; a dominator tree for one of the functions but not the other and without
> even
> +; a function analysis manager proxy for the SCC that things get merged
> into.
> +; Without proper handling when updating the call graph this will find a
> stale
> +; dominator tree.
> +
> + at test2_global = external global i32, align 4
> +
> +define void @test2_hoge(i1 (i32*)* %arg) {
> +; CHECK-LABEL: define void @test2_hoge(
> +bb:
> + %tmp2 = call zeroext i1 %arg(i32* @test2_global)
> +; CHECK: call zeroext i1 %arg(
> + br label %bb3
> +
> +bb3:
> + %tmp5 = call zeroext i1 %arg(i32* @test2_global)
> +; CHECK: call zeroext i1 %arg(
> + br i1 %tmp5, label %bb3, label %bb6
> +
> +bb6:
> + ret void
> +}
> +
> +define zeroext i1 @test2_widget(i32* %arg) {
> +; CHECK-LABEL: define zeroext i1 @test2_widget(
> +bb:
> + %tmp1 = alloca i8, align 1
> + %tmp2 = alloca i32, align 4
> + call void @test2_quux()
> +; CHECK-NOT: call
> +;
> +; CHECK: call zeroext i1 @test2_widget(i32* @test2_global)
> +; CHECK-NEXT: br label %[[NEW_BB:.*]]
> +;
> +; CHECK: [[NEW_BB]]:
> +; CHECK-NEXT: call zeroext i1 @test2_widget(i32* @test2_global)
> +;
> +; CHECK: {{.*}}:
> +
> + call void @test2_hoge.1(i32* %arg)
> +; CHECK-NEXT: call void @test2_hoge.1(
> +
> + %tmp4 = call zeroext i1 @test2_barney(i32* %tmp2)
> + %tmp5 = zext i1 %tmp4 to i32
> + store i32 %tmp5, i32* %tmp2, align 4
> + %tmp6 = call zeroext i1 @test2_barney(i32* null)
> + call void @test2_ham(i8* %tmp1)
> +; CHECK: call void @test2_ham(
> +
> + call void @test2_quux()
> +; CHECK-NOT: call
> +;
> +; CHECK: call zeroext i1 @test2_widget(i32* @test2_global)
> +; CHECK-NEXT: br label %[[NEW_BB:.*]]
> +;
> +; CHECK: [[NEW_BB]]:
> +; CHECK-NEXT: call zeroext i1 @test2_widget(i32* @test2_global)
> +;
> +; CHECK: {{.*}}:
> + ret i1 true
> +; CHECK-NEXT: ret i1 true
> +}
> +
> +define internal void @test2_quux() {
> +; CHECK-NOT: @test2_quux
> +bb:
> + call void @test2_hoge(i1 (i32*)* @test2_widget)
> + ret void
> +}
> +
> +declare void @test2_hoge.1(i32*)
> +
> +declare zeroext i1 @test2_barney(i32*)
> +
> +declare void @test2_ham(i8*)
>
> Modified: llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp?rev=307498&r1=307497&r2=307498&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp (original)
> +++ llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp Sun Jul 9
> 06:45:11 2017
> @@ -1277,9 +1277,10 @@ TEST(LazyCallGraphTest, InternalEdgeMuta
> // be invalidated.
> LazyCallGraph::SCC &AC = *CG.lookupSCC(A);
> LazyCallGraph::SCC &CC = *CG.lookupSCC(C);
> - auto InvalidatedSCCs = RC.switchInternalEdgeToCall(A, C);
> - ASSERT_EQ(1u, InvalidatedSCCs.size());
> - EXPECT_EQ(&AC, InvalidatedSCCs[0]);
> + EXPECT_TRUE(RC.switchInternalEdgeToCall(A, C,
> [&](ArrayRef<LazyCallGraph::SCC *> MergedCs) {
> + ASSERT_EQ(1u, MergedCs.size());
> + EXPECT_EQ(&AC, MergedCs[0]);
> + }));
> EXPECT_EQ(2, CC.size());
> EXPECT_EQ(&CC, CG.lookupSCC(A));
> EXPECT_EQ(&CC, CG.lookupSCC(C));
> @@ -1586,8 +1587,7 @@ TEST(LazyCallGraphTest, InternalRefEdgeT
>
> // Switch the ref edge from A -> D to a call edge. This should have no
> // effect as it is already in postorder and no new cycles are formed.
> - auto MergedCs = RC.switchInternalEdgeToCall(A, D);
> - EXPECT_EQ(0u, MergedCs.size());
> + EXPECT_FALSE(RC.switchInternalEdgeToCall(A, D));
> ASSERT_EQ(4, RC.size());
> EXPECT_EQ(&DC, &RC[0]);
> EXPECT_EQ(&BC, &RC[1]);
> @@ -1596,8 +1596,7 @@ TEST(LazyCallGraphTest, InternalRefEdgeT
>
> // Switch B -> C to a call edge. This doesn't form any new cycles but
> does
> // require reordering the SCCs.
> - MergedCs = RC.switchInternalEdgeToCall(B, C);
> - EXPECT_EQ(0u, MergedCs.size());
> + EXPECT_FALSE(RC.switchInternalEdgeToCall(B, C));
> ASSERT_EQ(4, RC.size());
> EXPECT_EQ(&DC, &RC[0]);
> EXPECT_EQ(&CC, &RC[1]);
> @@ -1605,9 +1604,10 @@ TEST(LazyCallGraphTest, InternalRefEdgeT
> EXPECT_EQ(&AC, &RC[3]);
>
> // Switch C -> B to a call edge. This forms a cycle and forces merging
> SCCs.
> - MergedCs = RC.switchInternalEdgeToCall(C, B);
> - ASSERT_EQ(1u, MergedCs.size());
> - EXPECT_EQ(&CC, MergedCs[0]);
> + EXPECT_TRUE(RC.switchInternalEdgeToCall(C, B,
> [&](ArrayRef<LazyCallGraph::SCC *> MergedCs) {
> + ASSERT_EQ(1u, MergedCs.size());
> + EXPECT_EQ(&CC, MergedCs[0]);
> + }));
> ASSERT_EQ(3, RC.size());
> EXPECT_EQ(&DC, &RC[0]);
> EXPECT_EQ(&BC, &RC[1]);
> @@ -1720,8 +1720,7 @@ TEST(LazyCallGraphTest, InternalRefEdgeT
> // Switch C3 -> B1 to a call edge. This doesn't form any new cycles but
> does
> // require reordering the SCCs in the face of tricky internal node
> // structures.
> - auto MergedCs = RC.switchInternalEdgeToCall(C3, B1);
> - EXPECT_EQ(0u, MergedCs.size());
> + EXPECT_FALSE(RC.switchInternalEdgeToCall(C3, B1));
> ASSERT_EQ(8, RC.size());
> EXPECT_EQ(&DC, &RC[0]);
> EXPECT_EQ(&B3C, &RC[1]);
> @@ -1852,10 +1851,12 @@ TEST(LazyCallGraphTest, InternalRefEdgeT
> // C F C | |
> // \ / \ / |
> // G G |
> - auto MergedCs = RC.switchInternalEdgeToCall(F, B);
> - ASSERT_EQ(2u, MergedCs.size());
> - EXPECT_EQ(&FC, MergedCs[0]);
> - EXPECT_EQ(&DC, MergedCs[1]);
> + EXPECT_TRUE(RC.switchInternalEdgeToCall(
> + F, B, [&](ArrayRef<LazyCallGraph::SCC *> MergedCs) {
> + ASSERT_EQ(2u, MergedCs.size());
> + EXPECT_EQ(&FC, MergedCs[0]);
> + EXPECT_EQ(&DC, MergedCs[1]);
> + }));
> EXPECT_EQ(3, BC.size());
>
> // And make sure the postorder was updated.
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170710/9e2b2961/attachment.html>
More information about the llvm-commits
mailing list