From 9586106cf5f4f7c16a365aec55b6255d4aa26db5 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Thu, 14 Sep 2017 08:33:57 +0000 Subject: [PATCH] [PM/CGSCC] Teach the CGSCC pass manager components to gracefully handle invalidated SCCs even when we do not have an updated SCC to redirect towards. This comes up in a fairly subtle and surprising circumstance: we need to have a connected but internal node in the call graph which later becomes a disconnected island, and then gets deleted. All of this needs to happen mid-CGSCC walk. Because it is disconnected, we have no way of computing a new "current" SCC when it gets deleted. Instead, we need to explicitly check for a deleted "current" SCC and bail out of the current CGSCC step. This will bubble all the way up to the post-order walk and then resume correctly. I've included minimal tests for this bug. The specific behavior matches something we've seen in the wild with the new PM combined with ThinLTO and sample PGO, but I've not yet confirmed whether this is the only issue there. llvm-svn: 313242 --- include/llvm/Analysis/CGSCCPassManager.h | 17 ++++-- lib/Analysis/CGSCCPassManager.cpp | 7 ++- test/Other/cgscc-disconnected-invalidation.ll | 54 +++++++++++++++++++ 3 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 test/Other/cgscc-disconnected-invalidation.ll diff --git a/include/llvm/Analysis/CGSCCPassManager.h b/include/llvm/Analysis/CGSCCPassManager.h index 4067c2a6904..8123cbad22f 100644 --- a/include/llvm/Analysis/CGSCCPassManager.h +++ b/include/llvm/Analysis/CGSCCPassManager.h @@ -429,13 +429,26 @@ public: UR.UpdatedC = nullptr; PreservedAnalyses PassPA = Pass.run(*C, CGAM, CG, UR); + // Update the SCC and RefSCC if necessary. + C = UR.UpdatedC ? UR.UpdatedC : C; + RC = UR.UpdatedRC ? UR.UpdatedRC : RC; + + // If the CGSCC pass wasn't able to provide a valid updated SCC, + // the current SCC may simply need to be skipped if invalid. + if (UR.InvalidatedSCCs.count(C)) { + DEBUG(dbgs() << "Skipping invalidated root or island SCC!\n"); + break; + } + // Check that we didn't miss any update scenario. + assert(C->begin() != C->end() && "Cannot have an empty SCC!"); + // We handle invalidating the CGSCC analysis manager's information // for the (potentially updated) SCC here. Note that any other SCCs // whose structure has changed should have been invalidated by // whatever was updating the call graph. This SCC gets invalidated // late as it contains the nodes that were actively being // processed. - CGAM.invalidate(*(UR.UpdatedC ? UR.UpdatedC : C), PassPA); + CGAM.invalidate(*C, PassPA); // Then intersect the preserved set so that invalidation of module // analyses will eventually occur when the module pass completes. @@ -450,8 +463,6 @@ public: // apart, at most converging on a DAG of single nodes. // FIXME: If we ever start having RefSCC passes, we'll want to // iterate there too. - RC = UR.UpdatedRC ? UR.UpdatedRC : RC; - C = UR.UpdatedC ? UR.UpdatedC : C; if (UR.UpdatedC) DEBUG(dbgs() << "Re-running SCC passes after a refinement of the " "current SCC: " diff --git a/lib/Analysis/CGSCCPassManager.cpp b/lib/Analysis/CGSCCPassManager.cpp index 9e6384437fb..cd115c97ef6 100644 --- a/lib/Analysis/CGSCCPassManager.cpp +++ b/lib/Analysis/CGSCCPassManager.cpp @@ -72,8 +72,13 @@ PassManagerbegin() != C->end() && "Cannot have an empty SCC!"); // Update the analysis manager as each pass runs and potentially diff --git a/test/Other/cgscc-disconnected-invalidation.ll b/test/Other/cgscc-disconnected-invalidation.ll new file mode 100644 index 00000000000..0567605fcf9 --- /dev/null +++ b/test/Other/cgscc-disconnected-invalidation.ll @@ -0,0 +1,54 @@ +; Test that patterns of transformations which disconnect a region of the call +; graph mid-traversal and then invalidate it function correctly. +; +; RUN: opt -S -passes='cgscc(inline,function(simplify-cfg))' < %s | FileCheck %s + +define internal void @test_scc_internal(i1 %flag) { +; CHECK-NOT: @test_scc_internal +entry: + br i1 %flag, label %then, label %else + +then: + call void @test_scc_internal(i1 false) + call void @test_scc_external() + br label %else + +else: + ret void +} + +define void @test_scc_external() { +; CHECK-LABEL: define void @test_scc_external() +entry: + call void @test_scc_internal(i1 false) + ret void +} + +define internal void @test_refscc_internal(i1 %flag, i8* %ptr) { +; CHECK-NOT: @test_refscc_internal +entry: + br i1 %flag, label %then, label %else + +then: + call void @test_refscc_internal(i1 false, i8* bitcast (i8* ()* @test_refscc_external to i8*)) + br label %else + +else: + ret void +} + +define i8* @test_refscc_external() { +; CHECK-LABEL: define i8* @test_refscc_external() +entry: + br i1 true, label %then, label %else +; CHECK-NEXT: entry: +; CHECK-NEXT: ret i8* null +; CHECK-NEXT: } +; CHECK-NOT: @test_refscc_internal + +then: + ret i8* null + +else: + ret i8* bitcast (void (i1, i8*)* @test_refscc_internal to i8*) +}