[Pipelines] Don't skip GlobalDCE in ThinLTO pre-link

[nikic]

Summary

GlobalDCE will only remove functions with available externally linkage if they are unreferenced. As such, I don't believe there is any problem with running this pass as part of the ThinLTO pre-link pipeline. It will only remove functions that are completely dead in that module, and I don't think there is any benefit to keeping them around for the post-link phase.

There is no compile-time impact from the additional pass.

This is a followup to one of the side discussions in D146776.

[tejohnson]

I was looking to see if I could determine why we wanted to keep unreferenced available_externally by looking at the history of this comment. It turns out this was only very recently moved into the module simplification pipeline in D145967. @aeubanks do you remember if there was a specific motivating case for the comment?

[aeubanks]

In D149446#4305330, @tejohnson wrote:

I was looking to see if I could determine why we wanted to keep unreferenced available_externally by looking at the history of this comment. It turns out this was only very recently moved into the module simplification pipeline in D145967. @aeubanks do you remember if there was a specific motivating case for the comment?

Hmm I swear I copied that comment from somewhere. That patch didn't actually change the pipeline, so maybe I incorrectly justified the state of things then. Anyway, lgtm

[mingmingl]

llvm/lib/Passes/PassBuilderPipelines.cpp

1118

This comment reminds me of another comment https://github.com/llvm/llvm-project/blob/4b80a8c72d3d0dba4ee2469471dfccb7e257b319/llvm/lib/Passes/PassBuilderPipelines.cpp#L983-l986, which points out in postlink (after function import) available_externally functions may appear unreferenced, if the function is referenced as virtual table indirect calls, so ICP pass runs before global-opt pass.

I don't know how functions become available_externally symbols in prelink (e.g., a small c++ example) stage, but wonder if the same 'seemingly unreferenced' but indeed referenced pattern is relevant here.

[tejohnson]

After chatting more with @mingmingl about this, I have a few theoretical concerns, both related to ICP as she pointed out, and also related to WPD since it looks like some vtable are emitted as available_externally by clang. Can you hold off on committing while we do some investigation?

[nikic]

In D149446#4306357, @tejohnson wrote:

After chatting more with @mingmingl about this, I have a few theoretical concerns, both related to ICP as she pointed out, and also related to WPD since it looks like some vtable are emitted as available_externally by clang. Can you hold off on committing while we do some investigation?

Sure!

[mingmingl]

llvm/lib/Passes/PassBuilderPipelines.cpp

1118

Update:

By flag-gate this pass pipeline change in an application binary (with thinlto and ifdo) and compare the remarks, the counter of remark lines is different (10-ish out of ~30,000 lines removed with prelink global-dce) for ICP. Repeat the flag gate method with WPD remarks gives a small (<0.5%?) counter difference (ignore hash difference in imported function names)

For ICP, the removed remarks is about a function defined in cpp header a.h takes a function pointer from b.cpp. Need to chase down a little bit to get a reduced test case.

[mingmingl]

llvm/lib/Passes/PassBuilderPipelines.cpp

1118

s/in an application binary/ flag gate the change in compiler to build an application binary with the flag on and off.

[mingmingl]

llvm/lib/Passes/PassBuilderPipelines.cpp

1118

Hi, the counter & remark differences turns out to be inside linkonce_odr functions and maps back to functions defined in header files (e.g., ref-counted class that invokes virtual destructors of the managed object, etc). After uniquifying the remarks (ignoring counts), the differences looks ok-ish (diffs come from hash suffixes of lto-imported functions). So the change should be benign.

Sorry for the delay, has been looking at this on-and-off.

[tejohnson]

lgtm

Thanks @mingmingl for digging into the effects!

llvm/lib/Passes/PassBuilderPipelines.cpp

PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   // Remove any dead arguments exposed by cleanups, constant folding globals,
   // and argument promotion.
   MPM.addPass(DeadArgumentEliminationPass());
 
   MPM.addPass(CoroCleanupPass());
 
   // Optimize globals now that functions are fully simplified.
   MPM.addPass(GlobalOptPass());
-
-  // Remove dead code, except in the ThinLTO pre-link pipeline where we may want
-  // to keep available_externally functions.
-  if (Phase != ThinOrFullLTOPhase::ThinLTOPreLink)
-    MPM.addPass(GlobalDCEPass());
 
   if (EnableMemProfiler && Phase != ThinOrFullLTOPhase::ThinLTOPreLink) {
     MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
     MPM.addPass(ModuleMemProfilerPass());
   }
 
   return MPM;
 }

llvm/test/Other/new-pm-thinlto-prelink-defaults.ll

 ; CHECK-O-NEXT: Running analysis: ShouldNotRunFunctionPassesAnalysis
 ; CHECK-O-NEXT: Running pass: CoroSplitPass
 ; CHECK-O-NEXT: Running pass: InvalidateAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
 ; CHECK-O-NEXT: Invalidating analysis: ShouldNotRunFunctionPassesAnalysis
 ; CHECK-O-NEXT: Invalidating analysis: InlineAdvisorAnalysis
 ; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
 ; CHECK-O-NEXT: Running pass: CoroCleanupPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
+; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-EXT: Running pass: {{.*}}::Bye
 ; CHECK-EP-OPT-EARLY-NEXT: Running pass: NoOpModulePass
 ; CHECK-EP-OPT-LAST-NEXT: Running pass: NoOpModulePass
 ; CHECK-O-NEXT:          Running pass: AnnotationRemarksPass on foo
 ; CHECK-O-NEXT: Running pass: CanonicalizeAliasesPass
 ; CHECK-O-NEXT: Running pass: NameAnonGlobalPass
 ; CHECK-O-NEXT: Running pass: PrintModulePass
 

llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll

 ; CHECK-O-NEXT: Running pass: CoroSplitPass
 ; CHECK-O-NEXT: Running pass: InvalidateAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
 ; CHECK-O-NEXT: Invalidating analysis: ShouldNotRunFunctionPassesAnalysis
 ; CHECK-O-NEXT: Invalidating analysis: InlineAdvisorAnalysis
 ; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
 ; CHECK-O-NEXT: Running pass: CoroCleanupPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis on bar
+; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-EXT: Running pass: {{.*}}::Bye
 ; CHECK-O-NEXT: Running pass: AnnotationRemarksPass on foo
 ; CHECK-O-NEXT: Running pass: CanonicalizeAliasesPass
 ; CHECK-O-NEXT: Running pass: NameAnonGlobalPass
 ; CHECK-O-NEXT: Running pass: PrintModulePass
 
 ; Make sure we get the IR back out without changes when we print the module.
 ; CHECK-O-LABEL: define void @foo(i32 %n) local_unnamed_addr {

llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll

 ; CHECK-O-NEXT: Running analysis: ShouldNotRunFunctionPassesAnalysis
 ; CHECK-O-NEXT: Running pass: CoroSplitPass
 ; CHECK-O-NEXT: Running pass: InvalidateAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
 ; CHECK-O-NEXT: Invalidating analysis: ShouldNotRunFunctionPassesAnalysis
 ; CHECK-O-NEXT: Invalidating analysis: InlineAdvisorAnalysis
 ; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
 ; CHECK-O-NEXT: Running pass: CoroCleanupPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
+; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-O-NEXT: Running pass: AnnotationRemarksPass on foo
 ; CHECK-O-NEXT: Running pass: CanonicalizeAliasesPass
 ; CHECK-O-NEXT: Running pass: NameAnonGlobalPass
 ; CHECK-O-NEXT: Running pass: PrintModulePass
 
 ; Make sure we get the IR back out without changes when we print the module.
 ; CHECK-O-LABEL: define void @foo(i32 %n) local_unnamed_addr
 ; CHECK-O-NEXT: entry: