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

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