Page MenuHomePhabricator

[legacyPM] Do not compute preserved analysis if there's no local change
ClosedPublic

Authored by serge-sans-paille on May 28 2020, 3:55 AM.

Details

Summary

It is my understanding that all analysis are preserved if there's no local change.
Skipping the dependency computation iimproves the performance when there's a lot of small functions, where only a few change happen.

Related to this thread https://lists.llvm.org/pipermail/llvm-dev/2020-May/141482.html, the performance change is not significant in terms of execution time, but the number instruction reported by perf does drop from 33.91B to 33.56B

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2020, 3:55 AM
nikic added a subscriber: nikic.May 28 2020, 11:12 AM
serge-sans-paille edited the summary of this revision. (Show Details)May 28 2020, 12:55 PM
lattner accepted this revision.May 28 2020, 2:29 PM

LGTM, but I'd appreciate comments from at least one other reviewer. Thanks!

This revision is now accepted and ready to land.May 28 2020, 2:29 PM
cuviper added a comment.EditedMay 28 2020, 4:53 PM

I applied this to the Rust fork to measure it in rust#72703, and here are the perf results. It looks like a reasonable gain -- no real effect on check/debug builds, but around 1% fewer instructions executed for optimized builds.

I can't vouch for correctness, except that it didn't cause any obvious problems in rustc itself, nor any test failures in the CI build.

Is this patch complete? I would have expected this change to break our pipeline testing approach, because those tests run on trivial functions where this would likely hide some/most places where analyses are recomputed.

foad added a subscriber: foad.May 29 2020, 5:36 AM

This makes me think that some passes are possibly not reporting changes correctly.

That would be my worry. As far as I know the boolean "changed" flag returned by runOn functions isn't really used for anything, and there is no verification to catch passes returning false when they actually did change something.

Perhaps if a pass returns false we should verify all analyses, rather than just the ones that the pass claims to preserve?

Perhaps if a pass returns false we should verify all analyses, rather than just the ones that the pass claims to preserve?

That sounds like a good idea! Do you think that should be guarded by NDEBUG or by EXPENSIVE_CHECK?

foad added a comment.Jun 1 2020, 1:51 AM

Perhaps if a pass returns false we should verify all analyses, rather than just the ones that the pass claims to preserve?

That sounds like a good idea! Do you think that should be guarded by NDEBUG or by EXPENSIVE_CHECK?

I don't know! I would suggest making it NDEBUG for simplicity, unless it proves to be very expensive.

https://reviews.llvm.org/D80916 proposes a few extra checks to ensure passes correctly report changes. I'll come back to this review once D80916 is settled.

foad added a comment.Jun 1 2020, 6:09 AM

https://reviews.llvm.org/D80916 proposes a few extra checks to ensure passes correctly report changes. I'll come back to this review once D80916 is settled.

FWIW I was thinking of something simpler like P8225 (almost completely untested).

foad added inline comments.Jun 1 2020, 6:13 AM
llvm/lib/IR/LegacyPassManager.cpp
1506–1507

Do the same for LoopPass, RegionPass and CallGraphSCCPass?

Perhaps if a pass returns false we should verify all analyses, rather than just the ones that the pass claims to preserve?

That sounds like a good idea! Do you think that should be guarded by NDEBUG or by EXPENSIVE_CHECK?

Most analysis verifications only seems to run with EXPENSIVE_CHECKS enabled today, so I'd stay consistent with that.

foad added a comment.Jul 24 2020, 6:52 AM

Are you planning to pick this up again now D80916 has landed? It LGTM modulo the comments that have already been made.

@nikic perhaps you could check whether the "text size changes for ReleaseLTO -g builds" are fixed now?

Are you planning to pick this up again now D80916 has landed? It LGTM modulo the comments that have already been made.

Sure!

@nikic perhaps you could check whether the "text size changes for ReleaseLTO -g builds" are fixed now?

@nikic: I'll be interested in that result too.

serge-sans-paille marked an inline comment as done.Jul 27 2020, 8:31 AM
serge-sans-paille added inline comments.
llvm/lib/IR/LegacyPassManager.cpp
1506–1507

AFAIU, these three passes are all handled by the FunctionPassmanager, so there shouldn't be anything to do.

foad added inline comments.Aug 4 2020, 8:24 AM
llvm/lib/IR/LegacyPassManager.cpp
1506–1507

LPPassManager::runOnFunction, RGPassManager::runOnFunction and CGPassManager::RunAllPassesOnSCC all call removeNotPreservedAnalysis unconditionally.

llvm/lib/IR/LegacyPassManager.cpp
1506–1507

Correct! I'll open a review for that. Thanks o/