[PATCH] D34728: [ThinkLTO] Invoke build(Thin)?LTOPreLinkDefaultPipeline.

[Tim Shen via Phabricator via cfe-commits]

timshen added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:804-807
+  case 0:
+    return PassBuilder::O0;
+
   case 1:
----------------
tejohnson wrote:
> chandlerc wrote:
> > Why is this change needed?
> 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.
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


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:893-894
       MPM.addPass(AlwaysInlinerPass());
+      if (IsThinLTO)
+        MPM.addPass(NameAnonGlobalPass());
     } else {
----------------
chandlerc wrote:
> 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?
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.


================
Comment at: llvm/test/Other/new-pm-thinlto-defaults.ll:157
 ; CHECK-PRELINK-O-NEXT: Running pass: GlobalOptPass
-; CHECK-PRELINK-O-NEXT: Running pass: NameAnonGlobalPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: PassManager<{{.*}}Module{{.*}}>
----------------
tejohnson wrote:
> chandlerc wrote:
> > tejohnson wrote:
> > > 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.
> > 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'
> > 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.
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.


https://reviews.llvm.org/D34728