Page MenuHomePhabricator

Change the phase ordering of SROA in the LTO to enable more cse opportunities
Needs ReviewPublic

Authored by jinlin on Jun 29 2021, 9:54 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Change the phase ordering of SROA in the LTO to enable more cse opportunities.

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 debian > LLVM.Other::opt-LTO-pipeline.ll
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/opt -enable-new-pm=0 -mtriple=x86_64-- -std-link-opts -debug-pass=Structure < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Other/opt-LTO-pipeline.ll -o /dev/null 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Other/opt-LTO-pipeline.ll
80 msx64 windows > LLVM.Other::opt-LTO-pipeline.ll
Script: -- : 'RUN: at line 1'; c:\ws\w4\llvm-project\premerge-checks\build\bin\opt.exe -enable-new-pm=0 -mtriple=x86_64-- -std-link-opts -debug-pass=Structure < C:\ws\w4\llvm-project\premerge-checks\llvm\test\Other\opt-LTO-pipeline.ll -o /dev/null 2>&1 | c:\ws\w4\llvm-project\premerge-checks\build\bin\filecheck.exe --check-prefix=CHECK C:\ws\w4\llvm-project\premerge-checks\llvm\test\Other\opt-LTO-pipeline.ll

Event Timeline

jinlin created this revision.Jun 29 2021, 9:54 AM
jinlin requested review of this revision.Jun 29 2021, 9:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2021, 9:54 AM
fhahn added a subscriber: fhahn.Jun 29 2021, 10:08 AM

This is only changing LTO side of obsolete pass manager.
If that is intentional, i'm not sure it's really worthwhile - it's obsolete pass manager is obsolete.

This is only changing LTO side of obsolete pass manager.
If that is intentional, i'm not sure it's really worthwhile - it's obsolete pass manager is obsolete.

When I added -flto, this pass will be invoked. The LTO is used in the release build at Uber. Why do you think it is obsolete?

This is only changing LTO side of obsolete pass manager.
If that is intentional, i'm not sure it's really worthwhile - it's obsolete pass manager is obsolete.

When I added -flto, this pass will be invoked. The LTO is used in the release build at Uber. Why do you think it is obsolete?

Because now the default is the New Pass Manager.

This is only changing LTO side of obsolete pass manager.
If that is intentional, i'm not sure it's really worthwhile - it's obsolete pass manager is obsolete.

When I added -flto, this pass will be invoked. The LTO is used in the release build at Uber. Why do you think it is obsolete?

Because now the default is the New Pass Manager.

Got you. The new pass manager is enabled by default starting from LLVM 12. I am using xcode 12.4 which uses prior version of llvm.

Got you. The new pass manager is enabled by default starting from LLVM 12. I am using xcode 12.4 which uses prior version of llvm.

The default of a specific Xcode version is not really relevant to upstream LLVM. If you build & use your own version of libLTO with Xcode's` ld, it would use the default of the upstream project anyways, right?

If this change has clear benefits, it should be used by both the new pass manager and the legacy one. Otherwise there may be additional regressions when the new pass manager is used.

To move forward with the patch, it would be good to include a phase-ordering test that shows the improvement of the new order. It would also be helpful if you could provide data on the impact in term of code-size and runtime performance on a larger set of programs/benchmarks to motivate the change.

I have three comments:

  1. I don't think we should do any old-pm specific changes. If this applies to the new-pm, then sure, old-pm can be changed to keep it in sync.
  2. I'm not sure why this is LTO-specific. Why should this not apply to the regular pipeline?
  3. I'm extremely unconvinced that simply moving SROA run like it is done in the patch now is okay.
llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
1084–1100

I may be reading this wrong, but to me it looks like we no longer do any SROA after running the inliner.
This function is called by populateLTOPassManager(), it does then call addLateLTOOptimizationPasses(),
but that one doesn't schedule any additional SROA runs.