[ThinkLTO] Invoke build(Thin)?LTOPreLinkDefaultPipeline.

[timshen]

Summary

Previously it doesn't actually invoke the designated new PM builder
functions.

This patch moves NameAnonGlobalPass out from PassBuilder, as Chandler points out that PassBuilder is used for non-O0 builds, and for optimizations only.

[timshen]

A question I have is that I don't know how to test this. Ideally we want -debug-pass-manager from opt, but that flag is not part of the LLVM libraries.

[chandlerc]

clang/lib/CodeGen/BackendUtil.cpp

804–807 ↗

(On Diff #104321)

Why is this change needed?

885–886 ↗

(On Diff #104321)

Keep this comment?

893–894 ↗

(On Diff #104321)

Why not a single addition of this pass at the end and a comment explaining taht regardless of optimization level this is required for ThinLTO?

[tejohnson]

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

157 ↗

(On Diff #104321)

Can this be changed to check for the pass being added in its new location, since it should still be invoked somewhere for ThinLTO? If this change means it is no longer added under options to set up the thinlto pipeline via opt, I'd prefer that we go back to adding this to the pipeline in buildThinLTOPreLinkDefaultPipeline in the non-O0 case.

[chandlerc]

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

157 ↗

(On Diff #104321)

It seems somewhat unfortunate to have a *semantic* requirement on a particular placement of this pass inside of the pipeline... Especially when the semantics pretty much only stem from C++. For example, an a language without anonymous globals, you might not want this pass when doing ThinLTO.

Note that you can exactly model the thing Clang is doing with opt even after this:

opt -passes='thinlto-pre-link<O3>,name-anon-globals'

[tejohnson]

In D34728#793131, @timshen wrote:

A question I have is that I don't know how to test this. Ideally we want -debug-pass-manager from opt, but that flag is not part of the LLVM libraries.

How about add a clang test that builds with "-mllvm -debug-pass=Structure" and -flto=thin, and check for the passes that are now expected to be added with this patch.

clang/lib/CodeGen/BackendUtil.cpp

804–807 ↗

(On Diff #104321)

I assume it is just cleanup since this isn't currently called under O0 (so it would hit the assert under the default switch case if we ever did). Not sure that is necessary though, up to you two.

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

157 ↗

(On Diff #104321)

For example, an a language without anonymous globals, you might not want this pass when doing ThinLTO.

Wouldn't it just be a no-op?

opt -passes='thinlto-pre-link<O3>,name-anon-globals'

True, it just seems less user-friendly. But since this is just for testing, I guess it doesn't matter much. So I am ok with your suggestion of pulling that out and doing a single call to this pass when in ThinLTO mode after setting up the pipelines.

[timshen]

Added -fexperimental-new-pass-manager=off/on/debug for printing debug information.

Added a Clang test.

Do tell if you want me to split this patch. I didn't, becuase then I don't have to write a test for each of them. :)

clang/lib/CodeGen/BackendUtil.cpp

804–807 ↗

(On Diff #104321)

Removed. I used to use it in prototyping, and forgot to remove it. It's always good to take a look at the patch after posting it, and I failed to do so. :P

893–894 ↗

(On Diff #104321)

I feel more readable this way. The structure has less "joint points", aka the if statements form a tree, rather than a DAG. I'm fine with the current duplication.

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

157 ↗

(On Diff #104321)

I added a clang test for checking everything in the pipeline.

In this LLVM test I used 'thinlto-pre-link<O3>,name-anon-globals', but the order of that pass changed.

[tejohnson]

In D34728#794508, @timshen wrote:

Added -fexperimental-new-pass-manager=off/on/debug for printing debug information.

Added a Clang test.

Do tell if you want me to split this patch. I didn't, becuase then I don't have to write a test for each of them. :)

I think they should be split into two patches, one for the option change and one for the thinLTO pipeline fixes. You could just put the new lto-newpm-pipeline.c test in with the option changes patch and commit it right after the thinlto pipeline fixes (or do it first and modify that test to add the thinlto passes when the pipeline fix goes in).

LGTM, but please wait to see if Chandler has any more comments.

clang/include/clang/Frontend/CodeGenOptions.h

92 ↗

(On Diff #104533)

Document

[timshen]

Splitted into two patches. I'll commit this one (ThinLTO change) without the test first, then commit the flag change with the Clang ThinLTO pipeline test.

[chandlerc]

LGTM as well.

[chandlerc]

Already said LGTM, please go ahead and submit. If tehre are further requsets, we can always amke follow-up changes.

cfe/trunk/lib/CodeGen/BackendUtil.cpp

 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/ObjCARC.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Scalar/GVN.h"
+#include "llvm/Transforms/Utils/NameAnonGlobals.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
 #include <memory>
 using namespace clang;
 using namespace llvm;
 
 namespace {
 
 // Default filename used for profile generation.
void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
   PB.registerCGSCCAnalyses(CGAM);
   PB.registerFunctionAnalyses(FAM);
   PB.registerLoopAnalyses(LAM);
   PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
 
   ModulePassManager MPM;
 
   if (!CodeGenOpts.DisableLLVMPasses) {
+    bool IsThinLTO = CodeGenOpts.EmitSummaryIndex;
+    bool IsLTO = CodeGenOpts.PrepareForLTO;
+
     if (CodeGenOpts.OptimizationLevel == 0) {
       // Build a minimal pipeline based on the semantics required by Clang,
       // which is just that always inlining occurs.
       MPM.addPass(AlwaysInlinerPass());
+      if (IsThinLTO)
+        MPM.addPass(NameAnonGlobalPass());
     } else {
-      // Otherwise, use the default pass pipeline. We also have to map our
-      // optimization levels into one of the distinct levels used to configure
-      // the pipeline.
+      // Map our optimization levels into one of the distinct levels used to
+      // configure the pipeline.
       PassBuilder::OptimizationLevel Level = mapToLevel(CodeGenOpts);
 
+      if (IsThinLTO) {
+        MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
+        MPM.addPass(NameAnonGlobalPass());
+      } else if (IsLTO) {
+        MPM = PB.buildLTOPreLinkDefaultPipeline(Level);
+      } else {
         MPM = PB.buildPerModuleDefaultPipeline(Level);
       }
     }
+  }
 
   // FIXME: We still use the legacy pass manager to do code generation. We
   // create that pass manager here and use it as needed below.
   legacy::PassManager CodeGenPasses;
   bool NeedCodeGen = false;
   Optional<raw_fd_ostream> ThinLinkOS;
 
   // Append any output we need to the pass manager.

llvm/trunk/lib/Passes/PassBuilder.cpp

PassBuilder::buildThinLTOPreLinkDefaultPipeline(OptimizationLevel Level,
   // less information than will be available later and it may grow functions in
   // ways that aren't beneficial.
   if (RunPartialInlining)
     MPM.addPass(PartialInlinerPass());
 
   // Reduce the size of the IR as much as possible.
   MPM.addPass(GlobalOptPass());
 
-  // Rename anon globals to be able to export them in the summary.
-  MPM.addPass(NameAnonGlobalPass());
-
   return MPM;
 }
 
 ModulePassManager
 PassBuilder::buildThinLTODefaultPipeline(OptimizationLevel Level,
                                          bool DebugLogging) {
   // FIXME: The summary index is not hooked in the new pass manager yet.
   // When it's going to be hooked, enable WholeProgramDevirt and LowerTypeTest

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

 ; The IR below was crafted so as:
 ; 1) To have a loop, so we create a loop pass manager
 ; 2) To be "immutable" in the sense that no pass in the standard
 ;    pipeline will modify it.
 ; Since no transformations take place, we don't expect any analyses
 ; to be invalidated.
 ; Any invalidation that shows up here is a bug, unless we started modifying
 ; the IR, in which case we need to make it immutable harder.
 ;
 ; Prelink pipelines:
 ; RUN: opt -disable-verify -debug-pass-manager \
-; RUN:     -passes='thinlto-pre-link<O1>' -S %s 2>&1 \
+; RUN:     -passes='thinlto-pre-link<O1>,name-anon-globals' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O1,CHECK-PRELINK-O,CHECK-PRELINK-O1
 ; RUN: opt -disable-verify -debug-pass-manager \
-; RUN:     -passes='thinlto-pre-link<O2>' -S  %s 2>&1 \
+; RUN:     -passes='thinlto-pre-link<O2>,name-anon-globals' -S  %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-PRELINK-O,CHECK-PRELINK-O2
 ; RUN: opt -disable-verify -debug-pass-manager \
-; RUN:     -passes='thinlto-pre-link<O3>' -S  %s 2>&1 \
+; RUN:     -passes='thinlto-pre-link<O3>,name-anon-globals' -S  %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O3,CHECK-PRELINK-O,CHECK-PRELINK-O3
 ; RUN: opt -disable-verify -debug-pass-manager \
-; RUN:     -passes='thinlto-pre-link<Os>' -S %s 2>&1 \
+; RUN:     -passes='thinlto-pre-link<Os>,name-anon-globals' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Os,CHECK-PRELINK-O,CHECK-PRELINK-Os
 ; RUN: opt -disable-verify -debug-pass-manager \
-; RUN:     -passes='thinlto-pre-link<Oz>' -S %s 2>&1 \
+; RUN:     -passes='thinlto-pre-link<Oz>,name-anon-globals' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-PRELINK-O,CHECK-PRELINK-Oz
 ;
 ; Postlink pipelines:
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto<O1>' -S %s 2>&1 \
 ; RUN:     | FileCheck %s --check-prefixes=CHECK-O,CHECK-O1,CHECK-POSTLINK-O,CHECK-POSTLINK-O1
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN:     -passes='thinlto<O2>' -S  %s 2>&1 \

 ; CHECK-O-NEXT: Running pass: ADCEPass
 ; CHECK-O-NEXT: Running analysis: PostDominatorTreeAnalysis
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-O-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-O-NEXT: Finished CGSCC pass manager run.
 ; CHECK-O-NEXT: Finished llvm::Module pass manager run.
 ; CHECK-PRELINK-O-NEXT: Running pass: GlobalOptPass
-; CHECK-PRELINK-O-NEXT: Running pass: NameAnonGlobalPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: PassManager<{{.*}}Module{{.*}}>
 ; CHECK-POSTLINK-O-NEXT: Starting llvm::Module pass manager run.
 ; CHECK-POSTLINK-O-NEXT: Running pass: GlobalOptPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: EliminateAvailableExternallyPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: ReversePostOrderFunctionAttrsPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}GlobalsAA
 ; CHECK-POSTLINK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
 ; CHECK-POSTLINK-O-NEXT: Starting llvm::Function pass manager run.

 ; CHECK-POSTLINK-O-NEXT: Running pass: LoopSinkPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: InstSimplifierPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-POSTLINK-O-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-POSTLINK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: ConstantMergePass
 ; CHECK-POSTLINK-O-NEXT: Finished llvm::Module pass manager run.
 ; CHECK-O-NEXT: Finished llvm::Module pass manager run.
+; CHECK-PRELINK-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:
 ; CHECK-O-NEXT:   br label %loop
 ; CHECK-O:      loop:
 ; CHECK-O-NEXT:   %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop ]