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

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/tools/opt/NewPMDriver.h:61
+                     bool EmitSummaryIndex, bool EmitModuleHash,
+                     Optional<tool_output_file *> ThinLTOBC);
 }
----------------
chandlerc wrote:
> timshen wrote:
> > 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. :)
> Ok, but this API is really, really confusing.
> 
> We should just add a new output kind to select between normal bitcode and thinlto bitcode. And then this can just be a pointer indicating if we want an additional thin link output.
> 
> But I'm happy to make that change in a follow-up (or for you to) as it'll need to change both PMs.
Done in the same patch. It's actually not that many changes to add an OutputKind for the new PM in opt. The old PM in opt doesn't even use such an enum (that's probably why they have these booleans :).


================
Comment at: llvm/tools/opt/opt.cpp:532-534
+    Optional<tool_output_file *> ThinLTOBC;
+    if (OutputThinLTOBC)
+      ThinLTOBC = ThinLinkOut.get();
----------------
chandlerc wrote:
> Inline this into the call?
> 
>   OutputThinLTOBC ? ThinLinkOut.get : None
> 
> (maybe with `{}` around it...)
It doesn't compile, because the two values provided by ?: should have the same type. :)

Also the Optional is gone. :)


https://reviews.llvm.org/D33525