Quuxplusone
commented
8 years ago It seems like this particular situation with jump-threading and globaldce is
actually quite rare. It looks like we don't hit this particular interaction in
test-suite when compiled with:
CMAKE_{C,CXX}_FLAGS:STRING=-Xclang "-newpm-
passes=module(function(sroa,instcombine,simplify-cfg,jump-
threading),globaldce,globalopt,deadargelim,function(sroa,instcombine,simplify-
cfg),require<profile-summary>,require<targetlibinfo>,cgscc(prune-
eh,inline,function-attrs,argpromotion,function(instcombine,simplify-
cfg,instcombine,jump-threading,simplify-cfg)))" -Xclang -newpm-aa-
pipeline=basic-aa,scoped-noalias-aa,type-based-aa,globals-aa
(these clang options are things I have locally and are analogous to the opt
options; I'll upstream them when I have a second)
I have to admit that it was not the original issue I hit in the wild which was
with the inliner. That one is *much* more likely and in fact it was the
original issue I ran into.
Below is a test case where globaldce deletes a function even though LazyValueInfo still has an AssertingVH to it. This analysis needs to be invalidated before the function is deleted. This is a symptom of a more general problem that I describe in more detail below (and describe 3 possible solutions).
$ cat foo.ll target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu"
declare i32 @bar()
define internal i32 @foo() { entry: %call4 = call i32 @bar() %cmp5 = icmp eq i32 %call4, 0 br i1 %cmp5, label %if.then6, label %if.end8
if.then6: ret i32 0
if.end8: ret i32 1 }
$ ~/pg/d+a/bin/opt foo.ll -passes='module(function(jump-threading),globaldce)' -debug-pass-manager -disable-output
Starting llvm::Module pass manager run. Running pass: VerifierPass on foo.ll Running analysis: VerifierAnalysis Running pass: PassManager on foo.ll
Starting llvm::Module pass manager run.
Running pass: ModuleToFunctionPassAdaptor<llvm::PassManager > on foo.ll
Running analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager, llvm::Module>
Starting llvm::Function pass manager run.
Running pass: JumpThreadingPass on foo
Running analysis: TargetLibraryAnalysis
Running analysis: LazyValueAnalysis
Running analysis: AssumptionAnalysis
Finished llvm::Function pass manager run.
Running pass: GlobalDCEPass on foo.ll
While deleting: label %entry
An asserting value handle still pointed to this value!
UNREACHABLE executed at /home/sean/pg/llvm/lib/IR/Value.cpp:800!
(bugzilla sometimes mangles things so here is a copy preserving the formatting: http://reviews.llvm.org/P6117)
This same sort of issue will apply generically to anyToPassAdaptor. In fact, I ran into this initially when building test-suite with http://reviews.llvm.org/D21921 applied (and using a fairly decent pipeline that contained
module(...,cgscc(...,inline,...function(...,jump-threading,...)),...)that happened to run into a similar issue)I see 3 solutions:
Have the ModuleToFunctionPassAdaptor invalidate all function analyses right after it finishes. This is safe and simple solution (that's the nature of a cache after all).
Use a CallbackVH to invalidate the analyses. This already is done by e.g. AssumptionCacheTracker::FunctionCallbackVH. This would mean that the AnalysisManager will need to hold CallbackVH's for invalidating corresponding analyses. For SCC's and Loop's we would need to add a similar destruction notification mechanism since they aren't Value's and so CallbackVH can't be used with them.
Require passes to manually invalidate analyses before deleting functions. This might entail a large maintenance and test burden and is generally error-prone. But if the number of passes that actually need special handling is fairly small, then it might be a preferable solution.
I'm in favor of 1. as a simple short-term solution. If that proves problematic (e.g. we measure significant performance to be gained by avoiding the analysis invalidation) then it would depend on how many places would need to be updated for 3. If only a relatively small and bounded set of passes would need special handling, then manual invalidation is probably best. Otherwise, option 2. would have less maintenance burden and less special code that needs to be added to passes.