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

[Chandler Carruth via cfe-commits]

Sorry, email misfire...

On Thu, Jun 29, 2017 at 4:15 PM Chandler Carruth via Phabricator via
llvm-commits  wrote:

> chandlerc added a comment.
>
> Already said LGTM, please go ahead and submit. If tehre are further
> requsets, we can always amke follow-up changes.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D34728
>
>
>
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D34728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tim Shen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL306756: [ThinkLTO] Invoke 
build(Thin)?LTOPreLinkDefaultPipeline. (authored by timshen).

Changed prior to commit:
  https://reviews.llvm.org/D34728?vs=104551=104767#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D34728

Files:
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  llvm/trunk/lib/Passes/PassBuilder.cpp
  llvm/trunk/test/Other/new-pm-thinlto-defaults.ll

Index: llvm/trunk/lib/Passes/PassBuilder.cpp
===
--- llvm/trunk/lib/Passes/PassBuilder.cpp
+++ llvm/trunk/lib/Passes/PassBuilder.cpp
@@ -758,9 +758,6 @@
   // 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;
 }
 
Index: llvm/trunk/test/Other/new-pm-thinlto-defaults.ll
===
--- llvm/trunk/test/Other/new-pm-thinlto-defaults.ll
+++ llvm/trunk/test/Other/new-pm-thinlto-defaults.ll
@@ -9,19 +9,19 @@
 ;
 ; Prelink pipelines:
 ; RUN: opt -disable-verify -debug-pass-manager \
-; RUN: -passes='thinlto-pre-link' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S  %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S  %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,name-anon-globals' -S %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-PRELINK-O,CHECK-PRELINK-Oz
 ;
 ; Postlink pipelines:
@@ -154,7 +154,6 @@
 ; 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
@@ -188,6 +187,7 @@
 ; 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.
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -54,6 +54,7 @@
 #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 
 using namespace clang;
@@ -881,17 +882,28 @@
   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);
 
-  MPM = PB.buildPerModuleDefaultPipeline(Level);
+  if (IsThinLTO) {
+MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
+MPM.addPass(NameAnonGlobalPass());
+  } else if (IsLTO) {
+MPM = PB.buildLTOPreLinkDefaultPipeline(Level);
+  } else {
+MPM = 

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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM as well.


https://reviews.llvm.org/D34728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tim Shen via Phabricator via cfe-commits]

timshen updated this revision to Diff 104551.
timshen added a comment.

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.


https://reviews.llvm.org/D34728

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-thinlto-defaults.ll

Index: llvm/test/Other/new-pm-thinlto-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-defaults.ll
@@ -9,19 +9,19 @@
 ;
 ; Prelink pipelines:
 ; RUN: opt -disable-verify -debug-pass-manager \
-; RUN: -passes='thinlto-pre-link' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S  %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S  %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,name-anon-globals' -S %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-PRELINK-O,CHECK-PRELINK-Oz
 ;
 ; Postlink pipelines:
@@ -154,7 +154,6 @@
 ; 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
@@ -188,6 +187,7 @@
 ; 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.
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -758,9 +758,6 @@
   // 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;
 }
 
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -54,6 +54,7 @@
 #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 
 using namespace clang;
@@ -881,17 +882,28 @@
   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);
 
-  MPM = PB.buildPerModuleDefaultPipeline(Level);
+  if (IsThinLTO) {
+MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
+MPM.addPass(NameAnonGlobalPass());
+  } else if (IsLTO) {
+MPM = PB.buildLTOPreLinkDefaultPipeline(Level);
+  } else {
+MPM = PB.buildPerModuleDefaultPipeline(Level);
+  }
 }
   }
 
___
cfe-commits mailing list

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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/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.




Comment at: clang/include/clang/Frontend/CodeGenOptions.h:92
 
+  enum NewPassManagerKind {
+NewPM_Off,

Document


https://reviews.llvm.org/D34728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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,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,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,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

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

[Tim Shen via Phabricator via cfe-commits]

timshen updated this revision to Diff 104533.
timshen marked 5 inline comments as done.
timshen added a comment.

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. :)


https://reviews.llvm.org/D34728

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CodeGenOptions.def
  clang/include/clang/Frontend/CodeGenOptions.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/lto-newpm-pipeline.c
  llvm/include/llvm/Option/ArgList.h
  llvm/lib/Option/ArgList.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-thinlto-defaults.ll

Index: llvm/test/Other/new-pm-thinlto-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-defaults.ll
@@ -9,19 +9,19 @@
 ;
 ; Prelink pipelines:
 ; RUN: opt -disable-verify -debug-pass-manager \
-; RUN: -passes='thinlto-pre-link' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S  %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S  %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,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' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link,name-anon-globals' -S %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-PRELINK-O,CHECK-PRELINK-Oz
 ;
 ; Postlink pipelines:
@@ -154,7 +154,6 @@
 ; 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
@@ -188,6 +187,7 @@
 ; 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.
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -751,9 +751,6 @@
   // 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;
 }
 
Index: llvm/lib/Option/ArgList.cpp
===
--- llvm/lib/Option/ArgList.cpp
+++ llvm/lib/Option/ArgList.cpp
@@ -95,21 +95,6 @@
   return std::vector(Values.begin(), Values.end());
 }
 
-void ArgList::AddLastArg(ArgStringList , OptSpecifier Id) const {
-  if (Arg *A = getLastArg(Id)) {
-A->claim();
-A->render(*this, Output);
-  }
-}
-
-void ArgList::AddLastArg(ArgStringList , OptSpecifier Id0,
- OptSpecifier Id1) const {
-  if (Arg *A = getLastArg(Id0, Id1)) {
-A->claim();
-A->render(*this, Output);
-  }
-}
-
 void ArgList::AddAllArgsExcept(ArgStringList ,
ArrayRef Ids,
ArrayRef ExcludeIds) const {
Index: llvm/include/llvm/Option/ArgList.h
===
--- llvm/include/llvm/Option/ArgList.h
+++ llvm/include/llvm/Option/ArgList.h
@@ -306,9 +306,13 @@
bool Default = true) const;
 
   /// AddLastArg - Render only the last argument match \p Id0, if present.
-  void AddLastArg(ArgStringList , OptSpecifier Id0) const;
-  void AddLastArg(ArgStringList , OptSpecifier Id0,
-  OptSpecifier Id1) const;
+  template 
+  void AddLastArg(ArgStringList , OptSpecifiers ...Ids) 

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

[Teresa Johnson via Phabricator via cfe-commits]

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,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,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

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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



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:
> 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,name-anon-globals'


https://reviews.llvm.org/D34728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.



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{{.*}}>

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.


https://reviews.llvm.org/D34728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:804-807
+  case 0:
+return PassBuilder::O0;
+
   case 1:

Why is this change needed?



Comment at: clang/lib/CodeGen/BackendUtil.cpp:885-886
 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());

Keep this comment?



Comment at: clang/lib/CodeGen/BackendUtil.cpp:893-894
   MPM.addPass(AlwaysInlinerPass());
+  if (IsThinLTO)
+MPM.addPass(NameAnonGlobalPass());
 } else {

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?


https://reviews.llvm.org/D34728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tim Shen via Phabricator via cfe-commits]

timshen added a comment.

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.


https://reviews.llvm.org/D34728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tim Shen via Phabricator via cfe-commits]

timshen created this revision.
Herald added subscribers: hiraditya, eraman, inglorion, mehdi_amini, sanjoy.

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


https://reviews.llvm.org/D34728

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-thinlto-defaults.ll


Index: llvm/test/Other/new-pm-thinlto-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-defaults.ll
@@ -154,7 +154,6 @@
 ; 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
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -751,9 +751,6 @@
   // 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;
 }
 
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -54,6 +54,7 @@
 #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 
 using namespace clang;
@@ -800,6 +801,9 @@
   default:
 llvm_unreachable("Invalid optimization level!");
 
+  case 0:
+return PassBuilder::O0;
+
   case 1:
 return PassBuilder::O1;
 
@@ -881,17 +885,26 @@
   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);
 
-  MPM = PB.buildPerModuleDefaultPipeline(Level);
+  if (IsThinLTO) {
+MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
+MPM.addPass(NameAnonGlobalPass());
+  } else if (IsLTO) {
+MPM = PB.buildLTOPreLinkDefaultPipeline(Level);
+  } else {
+MPM = PB.buildPerModuleDefaultPipeline(Level);
+  }
 }
   }
 


Index: llvm/test/Other/new-pm-thinlto-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-defaults.ll
@@ -154,7 +154,6 @@
 ; 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
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -751,9 +751,6 @@
   // 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;
 }
 
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -54,6 +54,7 @@
 #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 
 using namespace clang;
@@ -800,6 +801,9 @@
   default:
 llvm_unreachable("Invalid optimization level!");
 
+  case 0:
+return PassBuilder::O0;
+
   case 1:
 return PassBuilder::O1;
 
@@ -881,17 +885,26 @@
   ModulePassManager MPM;
 
   if