[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-30 Thread Tim Shen via Phabricator via cfe-commits 
timshen added inline comments.



Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:443
+  writeThinLTOBitcode(OS, ThinLinkOS,
+  [](Function ) -> AAResults & {
+return FAM.getResult(F);

chandlerc wrote:
> Are we unable to deduce the return type here?
No, because return type deduction uses "auto deduction" instead of "decltype 
deduction" - it deduces to a value type.



Comment at: llvm/tools/opt/NewPMDriver.h:61
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional ThinLTOBC);
 }

chandlerc wrote:
> timshen wrote:
> > chandlerc wrote:
> > > Why an optional pointer? Maybe just allow the pointer to be null if there 
> > > isn't a thin link output file?
> > There are 3 states:
> > 1) Use ThinLTOBitcodeWrite pass, controlled by OutputThinLTOBC, and the 
> > output file is not null.
> > 2) OutputThinLTOBC is true, but output file is null (don't write to the 
> > file).
> > 3) OutputThinLTOBC is false.
> > 
> > An optional nullable pointer, confusing as it might be (but I'm not sure 
> > how much), provides 3 states.
> > 
> > If we instead pass in a bool and a pointer, that's 4 states, one of which 
> > is invalid.
> Ok, but is #2 in your list actually useful? Maybe it is, but if it is then at 
> the very least the comment should make it super clear what is going on here. 
> Otherwise, I suspect many readers will assume than when the optional is 
> engaged the pointer isn't null as that's just too "obvious". 
#2 is consistent with opt's legacy pass manager behavior (search 
"createWriteThinLTOBitcodePass" in opt.cpp).

I added an emphasized "*nullable*" in the comment. :)


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-30 Thread Tim Shen via Phabricator via cfe-commits 
timshen updated this revision to Diff 100761.
timshen marked 12 inline comments as done.
timshen added a comment.

Updated based on the comments, and left out Clang changes for a separate patch.


https://reviews.llvm.org/D33525

Files:
  llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
  llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll
  llvm/tools/opt/NewPMDriver.cpp
  llvm/tools/opt/NewPMDriver.h
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -529,10 +529,13 @@
 // The user has asked to use the new pass manager and provided a pipeline
 // string. Hand off the rest of the functionality to the new code for that
 // layer.
-return runPassPipeline(argv[0], *M, TM.get(), Out.get(),
-   PassPipeline, OK, VK, PreserveAssemblyUseListOrder,
+Optional ThinLTOBC;
+if (OutputThinLTOBC)
+  ThinLTOBC = ThinLinkOut.get();
+return runPassPipeline(argv[0], *M, TM.get(), Out.get(), PassPipeline, OK,
+   VK, PreserveAssemblyUseListOrder,
PreserveBitcodeUseListOrder, EmitSummaryIndex,
-   EmitModuleHash)
+   EmitModuleHash, ThinLTOBC)
? 0
: 1;
   }
Index: llvm/tools/opt/NewPMDriver.h
===
--- llvm/tools/opt/NewPMDriver.h
+++ llvm/tools/opt/NewPMDriver.h
@@ -21,6 +21,8 @@
 #ifndef LLVM_TOOLS_OPT_NEWPMDRIVER_H
 #define LLVM_TOOLS_OPT_NEWPMDRIVER_H
 
+#include 
+
 namespace llvm {
 class StringRef;
 class LLVMContext;
@@ -47,13 +49,16 @@
 /// inclusion of the new pass manager headers and the old headers into the same
 /// file. It's interface is consequentially somewhat ad-hoc, but will go away
 /// when the transition finishes.
-bool runPassPipeline(StringRef Arg0, Module ,
- TargetMachine *TM, tool_output_file *Out,
- StringRef PassPipeline, opt_tool::OutputKind OK,
- opt_tool::VerifierKind VK,
+///
+/// ThinLTOBC indicates whether to use ThinLTO's bitcode writer, and if so,
+/// the *nullable* pointer points to the opened link file.
+bool runPassPipeline(StringRef Arg0, Module , TargetMachine *TM,
+ tool_output_file *Out, StringRef PassPipeline,
+ opt_tool::OutputKind OK, opt_tool::VerifierKind VK,
  bool ShouldPreserveAssemblyUseListOrder,
  bool ShouldPreserveBitcodeUseListOrder,
- bool EmitSummaryIndex, bool EmitModuleHash);
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional ThinLTOBC);
 }
 
 #endif
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
+#include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/Scalar/LoopPassManager.h"
 
 using namespace llvm;
@@ -47,13 +48,13 @@
 "pipeline for handling managed aliasing queries"),
cl::Hidden);
 
-bool llvm::runPassPipeline(StringRef Arg0, Module ,
-   TargetMachine *TM, tool_output_file *Out,
-   StringRef PassPipeline, OutputKind OK,
-   VerifierKind VK,
+bool llvm::runPassPipeline(StringRef Arg0, Module , TargetMachine *TM,
+   tool_output_file *Out, StringRef PassPipeline,
+   OutputKind OK, VerifierKind VK,
bool ShouldPreserveAssemblyUseListOrder,
bool ShouldPreserveBitcodeUseListOrder,
-   bool EmitSummaryIndex, bool EmitModuleHash) {
+   bool EmitSummaryIndex, bool EmitModuleHash,
+   Optional ThinLTOBC) {
   PassBuilder PB(TM);
 
   // Specially handle the alias analysis manager so that we can register
@@ -101,8 +102,13 @@
 PrintModulePass(Out->os(), "", ShouldPreserveAssemblyUseListOrder));
 break;
   case OK_OutputBitcode:
-MPM.addPass(BitcodeWriterPass(Out->os(), ShouldPreserveBitcodeUseListOrder,
-  EmitSummaryIndex, EmitModuleHash));
+if (ThinLTOBC)
+  MPM.addPass(ThinLTOBitcodeWriterPass(
+  Out->os(), *ThinLTOBC ? &(*ThinLTOBC)->os() : nullptr));
+else
+  MPM.addPass(BitcodeWriterPass(Out->os(),
+ShouldPreserveBitcodeUseListOrder,
+EmitSummaryIndex, EmitModuleHash));
 break;
   }
 
@@ -113,7 +119,10 @@

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-30 Thread Chandler Carruth via Phabricator via cfe-commits 
chandlerc added inline comments.



Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll:1
+; RUN: opt -passes='lto' -debug-pass-manager -thinlto-bc 
-thin-link-bitcode-file=%t2 -o %t %s 2>&1 | FileCheck %s --check-prefix=DEBUG_PM
+; RUN: llvm-bcanalyzer -dump %t2 | FileCheck %s --check-prefix=BITCODE

timshen wrote:
> chandlerc wrote:
> > Why run any passes here? 
> I saw that the runPassPipeline call is in  `if 
> (PassPipeline.getNumOccurrences() > 0) {`. I take that as "must specify 
> -passes" in order to run the new pass manager (which is created in 
> runPassPipeline). And specifiy an empty -passes cause an error message 
> "unable to parse pass pipeline description".
You can use '-passes=no-op-module' or something similar to at least minimize 
how much occurs using the new PM?



Comment at: llvm/tools/opt/NewPMDriver.h:61
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional ThinLTOBC);
 }

timshen wrote:
> chandlerc wrote:
> > Why an optional pointer? Maybe just allow the pointer to be null if there 
> > isn't a thin link output file?
> There are 3 states:
> 1) Use ThinLTOBitcodeWrite pass, controlled by OutputThinLTOBC, and the 
> output file is not null.
> 2) OutputThinLTOBC is true, but output file is null (don't write to the file).
> 3) OutputThinLTOBC is false.
> 
> An optional nullable pointer, confusing as it might be (but I'm not sure how 
> much), provides 3 states.
> 
> If we instead pass in a bool and a pointer, that's 4 states, one of which is 
> invalid.
Ok, but is #2 in your list actually useful? Maybe it is, but if it is then at 
the very least the comment should make it super clear what is going on here. 
Otherwise, I suspect many readers will assume than when the optional is engaged 
the pointer isn't null as that's just too "obvious". 


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-30 Thread Tim Shen via Phabricator via cfe-commits 
timshen added inline comments.



Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll:1
+; RUN: opt -passes='lto' -debug-pass-manager -thinlto-bc 
-thin-link-bitcode-file=%t2 -o %t %s 2>&1 | FileCheck %s --check-prefix=DEBUG_PM
+; RUN: llvm-bcanalyzer -dump %t2 | FileCheck %s --check-prefix=BITCODE

chandlerc wrote:
> Why run any passes here? 
I saw that the runPassPipeline call is in  `if 
(PassPipeline.getNumOccurrences() > 0) {`. I take that as "must specify 
-passes" in order to run the new pass manager (which is created in 
runPassPipeline). And specifiy an empty -passes cause an error message "unable 
to parse pass pipeline description".



Comment at: llvm/tools/opt/NewPMDriver.h:61
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional ThinLTOBC);
 }

chandlerc wrote:
> Why an optional pointer? Maybe just allow the pointer to be null if there 
> isn't a thin link output file?
There are 3 states:
1) Use ThinLTOBitcodeWrite pass, controlled by OutputThinLTOBC, and the output 
file is not null.
2) OutputThinLTOBC is true, but output file is null (don't write to the file).
3) OutputThinLTOBC is false.

An optional nullable pointer, confusing as it might be (but I'm not sure how 
much), provides 3 states.

If we instead pass in a bool and a pointer, that's 4 states, one of which is 
invalid.


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-26 Thread Chandler Carruth via Phabricator via cfe-commits 
chandlerc added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:913-914
+std::error_code EC;
+ThinLinkOS.emplace(CodeGenOpts.ThinLinkBitcodeFile, EC,
+   llvm::sys::fs::F_None);
+if (EC) {

The clang side of this should probably be a separate review on cfe-commits, but 
one drive-by coment.

I find this usage of emplace much harder to read than:

  ThinLinkOS.reset(raw_fd_ostream(CodeGenOpts.ThinLinkBitcodeFile, EC,
  llvm::sys::fs::F_None));

Unless we just can't do the above for some reason (it won't compile =[) I would 
prefer it.



Comment at: llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h:31-32
+public:
+  ThinLTOBitcodeWriterPass(raw_ostream , raw_ostream *ThinLinkOS)
+  : OS(OS), ThinLinkOS(ThinLinkOS) {}
+

Move (or add) a comment about this API to thea header? Notably, under what 
circumstances can `ThinLinkOS` be null? What happens if it is?



Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:9-12
 //
-// This pass prepares a module containing type metadata for ThinLTO by 
splitting
-// it into regular and thin LTO parts if possible, and writing both parts to
-// a multi-module bitcode file. Modules that do not contain type metadata are
-// written unmodified as a single module.
+// Pass implementation.
 //
 
//===--===//

Can just omit the added section I suspect.



Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:443
+  writeThinLTOBitcode(OS, ThinLinkOS,
+  [](Function ) -> AAResults & {
+return FAM.getResult(F);

Are we unable to deduce the return type here?



Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll:1
+; RUN: opt -passes='lto' -debug-pass-manager -thinlto-bc 
-thin-link-bitcode-file=%t2 -o %t %s 2>&1 | FileCheck %s --check-prefix=DEBUG_PM
+; RUN: llvm-bcanalyzer -dump %t2 | FileCheck %s --check-prefix=BITCODE

Why run any passes here? 



Comment at: llvm/tools/opt/NewPMDriver.h:61
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional ThinLTOBC);
 }

Why an optional pointer? Maybe just allow the pointer to be null if there isn't 
a thin link output file?



Comment at: llvm/tools/opt/opt.cpp:534
+if (OutputThinLTOBC)
+  ThinLTOBC.emplace(ThinLinkOut.get());
+if (!runPassPipeline(argv[0], *M, TM.get(), Out.get(), PassPipeline, OK, 
VK,

Maybe unnecessary based on the above comment, but generally I would just assign 
rather than using emplace.



Comment at: llvm/tools/opt/opt.cpp:541-542
+
+if (ThinLinkOut)
+  ThinLinkOut->keep();
+

We do `Out->keep()` in `runPassPipeline`, why not do this there as well?


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-26 Thread Tim Shen via Phabricator via cfe-commits 
timshen updated this revision to Diff 100488.
timshen added a comment.

Add opt support and llvm test.


https://reviews.llvm.org/D33525

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/thin_link_bitcode.c
  llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
  llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll
  llvm/tools/opt/NewPMDriver.cpp
  llvm/tools/opt/NewPMDriver.h
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -529,12 +529,19 @@
 // The user has asked to use the new pass manager and provided a pipeline
 // string. Hand off the rest of the functionality to the new code for that
 // layer.
-return runPassPipeline(argv[0], *M, TM.get(), Out.get(),
-   PassPipeline, OK, VK, PreserveAssemblyUseListOrder,
-   PreserveBitcodeUseListOrder, EmitSummaryIndex,
-   EmitModuleHash)
-   ? 0
-   : 1;
+Optional ThinLTOBC;
+if (OutputThinLTOBC)
+  ThinLTOBC.emplace(ThinLinkOut.get());
+if (!runPassPipeline(argv[0], *M, TM.get(), Out.get(), PassPipeline, OK, VK,
+ PreserveAssemblyUseListOrder,
+ PreserveBitcodeUseListOrder, EmitSummaryIndex,
+ EmitModuleHash, ThinLTOBC))
+  return 1;
+
+if (ThinLinkOut)
+  ThinLinkOut->keep();
+
+return 0;
   }
 
   // Create a PassManager to hold and optimize the collection of passes we are
Index: llvm/tools/opt/NewPMDriver.h
===
--- llvm/tools/opt/NewPMDriver.h
+++ llvm/tools/opt/NewPMDriver.h
@@ -21,6 +21,8 @@
 #ifndef LLVM_TOOLS_OPT_NEWPMDRIVER_H
 #define LLVM_TOOLS_OPT_NEWPMDRIVER_H
 
+#include 
+
 namespace llvm {
 class StringRef;
 class LLVMContext;
@@ -47,13 +49,16 @@
 /// inclusion of the new pass manager headers and the old headers into the same
 /// file. It's interface is consequentially somewhat ad-hoc, but will go away
 /// when the transition finishes.
-bool runPassPipeline(StringRef Arg0, Module ,
- TargetMachine *TM, tool_output_file *Out,
- StringRef PassPipeline, opt_tool::OutputKind OK,
- opt_tool::VerifierKind VK,
+///
+/// ThinLTOBC indicates whether to use ThinLTO's bitcode writer, and if so,
+/// the optional link file path.
+bool runPassPipeline(StringRef Arg0, Module , TargetMachine *TM,
+ tool_output_file *Out, StringRef PassPipeline,
+ opt_tool::OutputKind OK, opt_tool::VerifierKind VK,
  bool ShouldPreserveAssemblyUseListOrder,
  bool ShouldPreserveBitcodeUseListOrder,
- bool EmitSummaryIndex, bool EmitModuleHash);
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional ThinLTOBC);
 }
 
 #endif
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -29,6 +29,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
+#include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/Scalar/LoopPassManager.h"
 
 using namespace llvm;
@@ -47,13 +48,13 @@
 "pipeline for handling managed aliasing queries"),
cl::Hidden);
 
-bool llvm::runPassPipeline(StringRef Arg0, Module ,
-   TargetMachine *TM, tool_output_file *Out,
-   StringRef PassPipeline, OutputKind OK,
-   VerifierKind VK,
+bool llvm::runPassPipeline(StringRef Arg0, Module , TargetMachine *TM,
+   tool_output_file *Out, StringRef PassPipeline,
+   OutputKind OK, VerifierKind VK,
bool ShouldPreserveAssemblyUseListOrder,
bool ShouldPreserveBitcodeUseListOrder,
-   bool EmitSummaryIndex, bool EmitModuleHash) {
+   bool EmitSummaryIndex, bool EmitModuleHash,
+   Optional ThinLTOBC) {
   PassBuilder PB(TM);
 
   // Specially handle the alias analysis manager so that we can register
@@ -101,8 +102,12 @@
 PrintModulePass(Out->os(), "", ShouldPreserveAssemblyUseListOrder));
 break;
   case OK_OutputBitcode:
-MPM.addPass(BitcodeWriterPass(Out->os(), ShouldPreserveBitcodeUseListOrder,
-  EmitSummaryIndex, EmitModuleHash));
+if (ThinLTOBC)
+  MPM.addPass(ThinLTOBitcodeWriterPass(
+  Out->os(), *ThinLTOBC ? &(*ThinLTOBC)->os() : nullptr));
+else
+

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-26 Thread Chandler Carruth via Phabricator via cfe-commits 
chandlerc added a comment.

In https://reviews.llvm.org/D33525#766050, @timshen wrote:

> In https://reviews.llvm.org/D33525#764251, @chandlerc wrote:
>
> > (focusing on the LLVM side of this review for now)
> >
> > Can you add an LLVM-based test? Can you add this to 
> > `lib/Passes/PassRegistry.def`?
>
>
> Talked offline. Given the fact that BitcodeWriter (and possibly assembly 
> writer?) is not registered either, it seems to be a larger issue that's out 
> of the scope of this patch.


While *generic* testing is out of scope, I think you need at least *some* 
testing of this in this patch. It can be an option directly in the `llvm-lto` 
tool or a direct option in the `opt` tool itself, but we shouldn't add a bunch 
of code to LLVM that can only ever be exercised by using some other tool.


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-26 Thread Tim Shen via Phabricator via cfe-commits 
timshen marked an inline comment as done.
timshen added a comment.

In https://reviews.llvm.org/D33525#764251, @chandlerc wrote:

> (focusing on the LLVM side of this review for now)
>
> Can you add an LLVM-based test? Can you add this to 
> `lib/Passes/PassRegistry.def`?


Talked offline. Given the fact that BitcodeWriter (and possibly assembly 
writer?) is not registered either, it seems to be a larger issue that's out of 
the scope of this patch.


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-26 Thread Tim Shen via Phabricator via cfe-commits 
timshen updated this revision to Diff 100462.
timshen added a comment.

Change the test case.


https://reviews.llvm.org/D33525

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/thin_link_bitcode.c
  llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

Index: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
===
--- llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
+++ llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
@@ -7,13 +7,11 @@
 //
 //===--===//
 //
-// This pass prepares a module containing type metadata for ThinLTO by splitting
-// it into regular and thin LTO parts if possible, and writing both parts to
-// a multi-module bitcode file. Modules that do not contain type metadata are
-// written unmodified as a single module.
+// Pass implementation.
 //
 //===--===//
 
+#include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Analysis/BasicAliasAnalysis.h"
 #include "llvm/Analysis/ModuleSummaryAnalysis.h"
 #include "llvm/Analysis/ProfileSummaryInfo.h"
@@ -436,3 +434,15 @@
 raw_ostream *ThinLinkOS) {
   return new WriteThinLTOBitcode(Str, ThinLinkOS);
 }
+
+PreservedAnalyses
+llvm::ThinLTOBitcodeWriterPass::run(Module , ModuleAnalysisManager ) {
+  FunctionAnalysisManager  =
+  AM.getResult(M).getManager();
+  writeThinLTOBitcode(OS, ThinLinkOS,
+  [](Function ) -> AAResults & {
+return FAM.getResult(F);
+  },
+  M, (M));
+  return PreservedAnalyses::all();
+}
Index: llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
===
--- /dev/null
+++ llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
@@ -0,0 +1,39 @@
+//===- ThinLTOBitcodeWriter.h - Bitcode writing pass for ThinLTO --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This pass prepares a module containing type metadata for ThinLTO by splitting
+// it into regular and thin LTO parts if possible, and writing both parts to
+// a multi-module bitcode file. Modules that do not contain type metadata are
+// written unmodified as a single module.
+//
+//===--===//
+
+#ifndef LLVM_TRANSFORMS_IPO_THINLTOBITCODEWRITER_H
+#define LLVM_TRANSFORMS_IPO_THINLTOBITCODEWRITER_H
+
+#include 
+#include 
+
+namespace llvm {
+
+class ThinLTOBitcodeWriterPass
+: public PassInfoMixin {
+  raw_ostream 
+  raw_ostream *ThinLinkOS;
+
+public:
+  ThinLTOBitcodeWriterPass(raw_ostream , raw_ostream *ThinLinkOS)
+  : OS(OS), ThinLinkOS(ThinLinkOS) {}
+
+  PreservedAnalyses run(Module , ModuleAnalysisManager );
+};
+
+} // namespace llvm
+
+#endif
Index: clang/test/CodeGen/thin_link_bitcode.c
===
--- clang/test/CodeGen/thin_link_bitcode.c
+++ clang/test/CodeGen/thin_link_bitcode.c
@@ -1,6 +1,9 @@
 // RUN: %clang_cc1 -o %t -flto=thin -fthin-link-bitcode=%t.nodebug -triple x86_64-unknown-linux-gnu -emit-llvm-bc -debug-info-kind=limited %s
 // RUN: llvm-bcanalyzer -dump %t | FileCheck %s
 // RUN: llvm-bcanalyzer -dump %t.nodebug | FileCheck %s --check-prefix=NO_DEBUG
+// RUN: %clang_cc1 -o %t.newpm -flto=thin -fexperimental-new-pass-manager -fthin-link-bitcode=%t.newpm.nodebug -triple x86_64-unknown-linux-gnu -emit-llvm-bc -debug-info-kind=limited  %s
+// RUN: llvm-bcanalyzer -dump %t.newpm | FileCheck %s
+// RUN: llvm-bcanalyzer -dump %t.newpm.nodebug | FileCheck %s --check-prefix=NO_DEBUG
 int main (void) {
   return 0;
 }
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -49,6 +49,7 @@
 #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"
@@ -898,16 +899,32 @@
   // create that pass manager here and use it as needed below.
   legacy::PassManager CodeGenPasses;
   bool NeedCodeGen = false;
+  Optional ThinLinkOS;
 
   // Append any output we need to the pass manager.
   switch (Action) {
   case Backend_EmitNothing:
 break;
 
   case Backend_EmitBC:
-MPM.addPass(BitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists,
-

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-25 Thread Peter Collingbourne via Phabricator via cfe-commits 
pcc added inline comments.



Comment at: clang/test/CodeGen/thin_link_bitcode.c:5
+// RUN: %clang_cc1 -o %t -flto=thin -fexperimental-new-pass-manager 
-fthin-link-bitcode=%t.newpm -triple x86_64-unknown-linux-gnu -emit-llvm-bc %s
+// RUN: llvm-bcanalyzer -dump %t.newpm | FileCheck %s --check-prefix=NEW_PM
 int main (void) {

`s/NEW_PM/NO_DEBUG/` and remove line 12.

I think you also want to test the contents of `%t` here.


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-25 Thread Tim Shen via Phabricator via cfe-commits 
timshen added a comment.

In https://reviews.llvm.org/D33525#764251, @chandlerc wrote:

> (focusing on the LLVM side of this review for now)
>
> Can you add an LLVM-based test? Can you add this to 
> `lib/Passes/PassRegistry.def`?


I see that not all passes are registered (BitcodeWriterPass). Is it an 
oversight?

Does a pass have to be default constructible, in order to be registered easily?


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-25 Thread Chandler Carruth via Phabricator via cfe-commits 
chandlerc added a comment.

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to 
`lib/Passes/PassRegistry.def`?




Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:423
+
+class AARGetter {
+  FunctionAnalysisManager 

timshen wrote:
> mehdi_amini wrote:
> > Can't you do it with a lambda?
> I can, except that AAR needs to be allocated outside of the lambda, on the 
> stack:
> 
> Optional AAR;
> auto F = [](Function &) -> AAResults& { ... };
> 
> (AAR can't be captured by value, since AAResults is not copyable) this way 
> AAR out-lives the lambda, and I feel less readable.
You don't need to keep a copy though... You can directly expose the reference 
returned from the `FunctionAnalysisManager` -- that reference will stay alive 
until the result is invalidated. So this will become something more like:

  [](Function ) { return FAM.getResult(F); }


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-24 Thread Tim Shen via Phabricator via cfe-commits 
timshen added inline comments.



Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:423
+
+class AARGetter {
+  FunctionAnalysisManager 

mehdi_amini wrote:
> Can't you do it with a lambda?
I can, except that AAR needs to be allocated outside of the lambda, on the 
stack:

Optional AAR;
auto F = [](Function &) -> AAResults& { ... };

(AAR can't be captured by value, since AAResults is not copyable) this way AAR 
out-lives the lambda, and I feel less readable.


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-24 Thread Mehdi AMINI via Phabricator via cfe-commits 
mehdi_amini added inline comments.



Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:423
+
+class AARGetter {
+  FunctionAnalysisManager 

Can't you do it with a lambda?


https://reviews.llvm.org/D33525



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

[PATCH] D33525: [ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.

2017-05-24 Thread Tim Shen via Phabricator via cfe-commits 
timshen created this revision.
Herald added subscribers: eraman, inglorion, Prazek, mehdi_amini.

Also see https://reviews.llvm.org/D33429 for other ThinLTO + New PM related 
changes.


https://reviews.llvm.org/D33525

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/CodeGen/thin_link_bitcode.c
  llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
  llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

Index: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
===
--- llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
+++ llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
@@ -7,13 +7,11 @@
 //
 //===--===//
 //
-// This pass prepares a module containing type metadata for ThinLTO by splitting
-// it into regular and thin LTO parts if possible, and writing both parts to
-// a multi-module bitcode file. Modules that do not contain type metadata are
-// written unmodified as a single module.
+// Pass implementation.
 //
 //===--===//
 
+#include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Analysis/BasicAliasAnalysis.h"
 #include "llvm/Analysis/ModuleSummaryAnalysis.h"
 #include "llvm/Analysis/ProfileSummaryInfo.h"
@@ -421,6 +419,21 @@
 AU.addRequired();
   }
 };
+
+class AARGetter {
+  FunctionAnalysisManager 
+  Optional AAR;
+
+public:
+  AARGetter(FunctionAnalysisManager ) : AM(AM) {}
+
+  AAResults ()(Function ) {
+if (!AAR)
+  AAR.emplace(AAManager().run(F, AM));
+return *AAR;
+  }
+};
+
 } // anonymous namespace
 
 char WriteThinLTOBitcode::ID = 0;
@@ -436,3 +449,13 @@
 raw_ostream *ThinLinkOS) {
   return new WriteThinLTOBitcode(Str, ThinLinkOS);
 }
+
+PreservedAnalyses
+llvm::ThinLTOBitcodeWriterPass::run(Module , ModuleAnalysisManager ) {
+  writeThinLTOBitcode(
+  OS, ThinLinkOS,
+  AARGetter(
+  AM.getResult(M).getManager()),
+  M, (M));
+  return PreservedAnalyses::all();
+}
Index: llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
===
--- /dev/null
+++ llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
@@ -0,0 +1,39 @@
+//===- ThinLTOBitcodeWriter.h - Bitcode writing pass for ThinLTO --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This pass prepares a module containing type metadata for ThinLTO by splitting
+// it into regular and thin LTO parts if possible, and writing both parts to
+// a multi-module bitcode file. Modules that do not contain type metadata are
+// written unmodified as a single module.
+//
+//===--===//
+
+#ifndef LLVM_TRANSFORMS_IPO_THINLTOBITCODEWRITER_H
+#define LLVM_TRANSFORMS_IPO_THINLTOBITCODEWRITER_H
+
+#include 
+#include 
+
+namespace llvm {
+
+class ThinLTOBitcodeWriterPass
+: public PassInfoMixin {
+  raw_ostream 
+  raw_ostream *ThinLinkOS;
+
+public:
+  ThinLTOBitcodeWriterPass(raw_ostream , raw_ostream *ThinLinkOS)
+  : OS(OS), ThinLinkOS(ThinLinkOS) {}
+
+  PreservedAnalyses run(Module , ModuleAnalysisManager );
+};
+
+} // namespace llvm
+
+#endif
Index: clang/test/CodeGen/thin_link_bitcode.c
===
--- clang/test/CodeGen/thin_link_bitcode.c
+++ clang/test/CodeGen/thin_link_bitcode.c
@@ -1,9 +1,12 @@
 // RUN: %clang_cc1 -o %t -flto=thin -fthin-link-bitcode=%t.nodebug -triple x86_64-unknown-linux-gnu -emit-llvm-bc -debug-info-kind=limited %s
 // RUN: llvm-bcanalyzer -dump %t | FileCheck %s
 // RUN: llvm-bcanalyzer -dump %t.nodebug | FileCheck %s --check-prefix=NO_DEBUG
+// RUN: %clang_cc1 -o %t -flto=thin -fexperimental-new-pass-manager -fthin-link-bitcode=%t.newpm -triple x86_64-unknown-linux-gnu -emit-llvm-bc %s
+// RUN: llvm-bcanalyzer -dump %t.newpm | FileCheck %s --check-prefix=NEW_PM
 int main (void) {
   return 0;
 }
 
 // CHECK: COMPILE_UNIT
 // NO_DEBUG-NOT: COMPILE_UNIT
+// NEW_PM-NOT: COMPILE_UNIT
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -49,6 +49,7 @@
 #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"
@@ -898,16 +899,33 @@
   // create that pass manager here and use it as needed below.
   legacy::PassManager