Page MenuHomePhabricator

Turn on the new pass manager by default
ClosedPublic

Authored by aeubanks on Jan 25 2021, 11:01 AM.

Details

Summary

This turns on the new pass manager by default for the optimization pipeline in
Clang and ThinLTO in various LLD backends. This also makes uses of `opt
-instcombine` use the new pass manager (unless specifically opted out).

This does not affect the backend target-dependent codegen pipeline.

If this causes regressions, you can opt out of the new pass manager
either via the -DENABLE_EXPERIMENTAL_NEW_PASS_MANAGER=OFF CMake flag
while building LLVM, or via various compiler flags, e.g.
-flegacy-pass-manager for Clang or -Wl,--lto-legacy-pass-manager for
ELF LLD. Please file bugs for any regressions.

Major differences:

  • The inliner works slightly differently
  • -O1 does some amount of inlining
  • LCSSA and LoopSimplify are run before all loop passes
  • Loop unswitching is implemented slightly differently
  • A new SpeculateAroundPHIs pass is added to the pipeline

https://lists.llvm.org/pipermail/llvm-dev/2021-January/148098.html

Diff Detail

Event Timeline

aeubanks created this revision.Jan 25 2021, 11:01 AM
aeubanks requested review of this revision.Jan 25 2021, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2021, 11:01 AM
aeubanks retitled this revision from Turn on the new pass manager by default to [WIP] Turn on the new pass manager by default.Jan 25 2021, 11:02 AM
fhahn added a subscriber: fhahn.Jan 25 2021, 11:34 AM
uenoku added a subscriber: uenoku.Jan 26 2021, 9:18 AM
dmgreen added a subscriber: dmgreen.Feb 1 2021, 4:07 AM
aeubanks retitled this revision from [WIP] Turn on the new pass manager by default to Turn on the new pass manager by default.Feb 2 2021, 10:13 AM
aeubanks edited the summary of this revision. (Show Details)
aeubanks edited the summary of this revision. (Show Details)Feb 2 2021, 10:32 AM

I'll submit this tomorrow if there are no objections

MaskRay accepted this revision.Feb 2 2021, 11:02 AM
This revision is now accepted and ready to land.Feb 2 2021, 11:02 AM
ychen accepted this revision.Feb 2 2021, 11:40 AM

LGTM.

Actually, I may have to push this back to investigate the failures coming out of the presubmit

openmp libarcher tests failed for unrelated reasons. They triggered a clang crash. I noticed the issues in my other patches.

asbirlea accepted this revision.Feb 2 2021, 2:21 PM
echristo accepted this revision.Feb 2 2021, 8:42 PM
echristo added a reviewer: tstellar.

Adding Tom here as release manager so they know.

Lots of discussion over the years on doing this and as far as I know we've gotten all of the items talked about on llvm-dev. LGTM. Thanks for pushing through with this!

Is this something we want to try to pull into the release/12.x branch?

Agreed with Eric, this definitely shouldn't go into 12.x.

This revision was landed with ongoing or failed builds.Feb 3 2021, 2:38 PM
This revision was automatically updated to reflect the committed changes.
dmajor added a subscriber: dmajor.Feb 3 2021, 6:48 PM

In the context of https://bugs.llvm.org/show_bug.cgi?id=49606, I realized that most developers are probably still using the old pass manager. That issue only repros with the new PM, and @jdoerfert was using the legacy PM. I checked my local build dir, and observed this line in it:

ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER:BOOL=FALSE

One way to deal with this would be to deprecate the legacy spelling of ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER and switch to LLVM_ENABLE_NEW_PASS_MANAGER. If we don't do this, we'll have to make a PSA about clearing CMakeCache.txt anyway, so I think it's probably worth it.

In D95380#2632044, @rnk wrote:

In the context of https://bugs.llvm.org/show_bug.cgi?id=49606, I realized that most developers are probably still using the old pass manager. That issue only repros with the new PM, and @jdoerfert was using the legacy PM. I checked my local build dir, and observed this line in it:

ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER:BOOL=FALSE

One way to deal with this would be to deprecate the legacy spelling of ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER and switch to LLVM_ENABLE_NEW_PASS_MANAGER. If we don't do this, we'll have to make a PSA about clearing CMakeCache.txt anyway, so I think it's probably worth it.

I'd prefer not to rename the variable, since it was used in a bunch of communication, and updating bots is a pain. A PSA sounds good though.