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

[Chandler Carruth via Phabricator via llvm-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 &OS, 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,
+                      [&FAM](Function &F) -> AAResults & {
+                        return FAM.getResult<AAManager>(F);
----------------
Are we unable to deduce the return type here?


================
Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll:1
+; RUN: opt -passes='lto<O2>' -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<tool_output_file *> 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