tejohnson added a comment.
In https://reviews.llvm.org/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.
================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:804-807
+ case 0:
+ return PassBuilder::O0;
+
case 1:
----------------
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.
================
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{{.*}}>
----------------
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.
https://reviews.llvm.org/D34728
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
