[PATCH] D28997: [LTO] Teach lib/LTO about the new pass manager

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/LTO/LTOBackend.cpp:248
+  if (Conf.OptUseNewPM || !Conf.OptPipeline.empty()) {
     // FIXME: Plumb the combined index into the new pass manager.
+    if (Conf.OptPipeline.empty())
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > davide wrote:
> > > mehdi_amini wrote:
> > > > In the meantime, we shouldn't accept the combination ThinLTO+Conf.OptUseNewPM.
> > > Indeed. Are you fine with this being a fatal error for the time being as the configuration is forbidden (similarly to what we do around line 212)?
> > > Otherwise I can just change the return value to std::pair<Error, bool> or return Error and pass bool as argument by reference, or whatever you like better.
> > I'm fine with a report_fatal_error for this.
> > 
> The issue for ThinLTO seems larger than just not having the summary plumbed through (it isn't being used yet in the new pass manager pipeline set up in D28996  in any case, since the LowerTypeTestsPass is currently disabled there). The bigger issue is that a new pass manager pipeline for ThinLTO (analogous to the one for LTO added in D28996) needs to be set up. Can you add a FIXME by the fatal error about this?
Could be rewritten more simply as:
```
if (!Conf.OptPipeline.empty())
  runNewPMCustomPasses
else if (LTOUseNewPM)
  runNewPMPasses
else
  runOldPMPasses
```
?


================
Comment at: lib/LTO/LTOBackend.cpp:45
 
+cl::opt<bool> LTOUseNewPM("lto-use-new-pm",
+                          cl::desc("Run passes using the new pass manager"),
----------------
`static`?


https://reviews.llvm.org/D28997