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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Still not really happy with htis API, but let's fix it in a follow-up as it will grow the scope and much of the problem was already there (and the API is fairly narrow in scope).



================
Comment at: llvm/tools/opt/NewPMDriver.h:61
+                     bool EmitSummaryIndex, bool EmitModuleHash,
+                     Optional<tool_output_file *> ThinLTOBC);
 }
----------------
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.


================
Comment at: llvm/tools/opt/opt.cpp:532-534
+    Optional<tool_output_file *> ThinLTOBC;
+    if (OutputThinLTOBC)
+      ThinLTOBC = ThinLinkOut.get();
----------------
Inline this into the call?

  OutputThinLTOBC ? ThinLinkOut.get : None

(maybe with `{}` around it...)


https://reviews.llvm.org/D33525