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