[PATCH] D86967: [PassManager] Move load/store motion pass after DSE in LTO pipeline.

Wed Sep 2 03:46:54 PDT 2020

fhahn added a comment.

In D86967#2250810 <https://reviews.llvm.org/D86967#2250810>, @asbirlea wrote:

> The idea to have all passes in sequence preserved and use MemorySSA is great. I'm wondering if changing the pass order affects run time (due to potential missed optimizations).
> On the other hand, this only affects the LPM, so it may not be as critical due to the plan to switch to the NPM.

This patch does not cause any binary changes for -O3 -flto on X86 for MultiSource/SPEC2000/SPEC2006, so it seems like the optimization behavior does not really change for that test set. Looking at the pass, it seems like it is relatively limited in the transformation it applies and should not be impacted a lot by the slight change of placement.

> The NPM has the passes that take advantage of MSSA combined as DSE+LICM (https://github.com/llvm-mirror/llvm/blob/master/lib/Passes/PassBuilder.cpp#L525) and GVN+MemCpyOpt (https://github.com/llvm-mirror/llvm/blob/master/lib/Passes/PassBuilder.cpp#L504), and GVN+MemCpyOpt+DSE for LTO (https://github.com/llvm-mirror/llvm/blob/master/lib/Passes/PassBuilder.cpp#L1303)
> So all cases should be covered by the other "preserve MSSA" patches.

Yes, for the regular pipeline DSE+LICM are combined with the NPM (same as with LPM). One unfortunate property of the NPM LTO pipeline is that LICM is not run (there's a FIXME), so there is no actual users of MemorySSA close-by, so even though we run GVN+MemCpyOpt+DSE and we could preserve MSSA it won't help much I think, because there is no 'real' consumer at the beginning (with LPM, we run LICM there during LTO).

> I'll follow up on the llvmdev@ thread regarding testing for the NPM.

I think the only major difference is that we currently have to construct MSSA only for DSE in the NPM LTO pipeline. That is unfortunate (and will be noticeable in terms of compile-time), but this problem will go away when either NewGVN or LICM is run as it seems expected. from the comments.

In D86967#2250849 <https://reviews.llvm.org/D86967#2250849>, @tejohnson wrote:

> In D86967#2250685 <https://reviews.llvm.org/D86967#2250685>, @xbolva00 wrote:
>
>> cc @tejohnson
>
> For the NPM there is a way to invoke the various LTO pass pipelines via opt, and I know there are several tests that do test the pass ordering out of that. For the old PM, though, the only way I could see in opt.cpp to get LTO passes is via the -std-link-opts opt flag, but it doesn't look like any tests actually use that option! Given the upcoming switch to the NPM I'm not sure if it is worth adding a bunch of old PM LTO pipeline tests at this point.

Right, the NPM order does not change. I don't think it's worth adding new options to print the LPM pipelines for a minor change such as this one.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86967