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