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

[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<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
----------------
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<tool_output_file *> 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