[PATCH] D96780: [NPM][LTO] Update buildLTODefaultPipeline to be more in-line with the old pass manager

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D96780#2566421 <https://reviews.llvm.org/D96780#2566421>, @dmgreen wrote:

> In D96780#2566399 <https://reviews.llvm.org/D96780#2566399>, @ychen wrote:
>
>> It would be great to have some compile-time impact data before landing (https://llvm-compile-time-tracker.com/, clang self-host, etc.). This also depends on the potential performance gain on average. Just want to make sure we make the appropriate tradeoff.
>
> "1350%" :)
>
> This is only trying to get us back to where we were before, before all the regressions from the new pass manager. Whilst I can agree that compile time is important, this was a pretty significant set of regressions for us.

+1. From the FIXMEs it appears as if the omission of LoopVectorize. & co was not intentional, but because of initial bugs/limitations, so enabling them even if it impacts compile-time seems appropriate to me. This will help making the transition as smooth as possible and will avoid us having to track down/fix obvious regressions caused by big differences in the pipeline.

I think if we want to drastically adjust the pipeline, that should be done well after we ironed out the issues caused alone by the NewPM switch.



================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1740
   // Nuke dead stores.
   MainFPM.addPass(DSEPass());
+  // FIXME: once we provide support for enabling MLSM, add it here.
----------------
dmgreen wrote:
> dmgreen wrote:
> > fhahn wrote:
> > > It looks like there are a few other passes missing here, like `LoopDeletion` and `MergedLoadStoreMotionPass`, and conditionally `LoopInterchange`/`ConstraintElimination`. Should they be added as well? 
> > I was trying to keep this simple, to fix the regressions sooner rather than later. I'll see what I can do to add a few others.
> LoopInterchange I've not added because it's not anywhere in the NPM yet. It can be added when it's added to the main optimization passes. The others should be here now.
Oh right, we don't have to add them all in one go.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96780/new/

https://reviews.llvm.org/D96780