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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
