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

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:443
+  writeThinLTOBitcode(OS, ThinLinkOS,
+                      [&FAM](Function &F) -> AAResults & {
+                        return FAM.getResult<AAManager>(F);
----------------
chandlerc wrote:
> Are we unable to deduce the return type here?
No, because return type deduction uses "auto deduction" instead of "decltype deduction" - it deduces to a value type.


================
Comment at: llvm/tools/opt/NewPMDriver.h:61
+                     bool EmitSummaryIndex, bool EmitModuleHash,
+                     Optional<tool_output_file *> ThinLTOBC);
 }
----------------
chandlerc wrote:
> 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". 
#2 is consistent with opt's legacy pass manager behavior (search "createWriteThinLTOBitcodePass" in opt.cpp).

I added an emphasized "*nullable*" in the comment. :)


https://reviews.llvm.org/D33525