[PATCH] D102002: [PassManager] unify vector passes between regular and LTO pipelines

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1263-1268
   FPM.addPass(SimplifyCFGPass(SimplifyCFGOptions()
                                   .forwardSwitchCondToPhi(true)
                                   .convertSwitchToLookupTable(true)
                                   .needCanonicalLoops(false)
                                   .hoistCommonInsts(true)
                                   .sinkCommonInsts(true)));
----------------
vzakhari wrote:
> spatel wrote:
> > spatel wrote:
> > > vzakhari wrote:
> > > > The code did not look like this at the place where https://reviews.llvm.org/rGfefcb1f878c2dad435af604955661ca02a5302de inserted a call to `addVectorPasses(... , /* IsLTO */ true)`.  It was only `.hoistCommonInsts(true)` - was this an expected change in behavior?
> > > No, that was a mistake. Thank you for spotting it!
> > > AFAIK, there's no way to show that difference in a pass manager unit test, so that's why it slipped through.
> > > Did you notice that by inspection or benchmark regression? If the latter, it would be great if we can reduce a test for it.
> > > Let me know if I should revert or update the earlier commit.
> > I went ahead and reverted the earlier change since it was clearly not NFC as advertised.
> I spotted it just during an inspection of a downstream conflict.  Sorry for the late reply, I do not think you had to revert it :)
No problem - this is going to need the smallest changes per step to ensure I'm not breaking things. :)
Thank you for reviewing the diffs!


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

https://reviews.llvm.org/D102002