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

[serge-sans-paille]

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

[lattner]

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

[cuviper]

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.

[nikic]

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.

[nikic]

CTMark numbers: http://llvm-compile-time-tracker.com/compare.php?from=9c52422cd83137a55e9d198bf123982c040b2e99&to=3277ba4fa8cae4e276482889a99757261264410e&stat=instructions This is a 0.4% improvement for -O3.

However, there are some text size changes for ReleaseLTO -g builds: http://llvm-compile-time-tracker.com/compare.php?from=9c52422cd83137a55e9d198bf123982c040b2e99&to=3277ba4fa8cae4e276482889a99757261264410e&stat=size-text This makes me think that some passes are possibly not reporting changes correctly.

[foad]

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?

[serge-sans-paille]

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]

In D80707#2065728, @serge-sans-paille wrote:

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.

[serge-sans-paille]

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]

In D80707#2066005, @serge-sans-paille wrote:

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]

llvm/lib/IR/LegacyPassManager.cpp

1617–1619

Do the same for LoopPass, RegionPass and CallGraphSCCPass?

[nhaehnle]

In D80707#2065728, @serge-sans-paille wrote:

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]

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?

[serge-sans-paille]

In D80707#2172200, @foad wrote:

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.

[nikic]

New compile-time numbers: https://llvm-compile-time-tracker.com/compare.php?from=183342c0a9850e60dd7a004b651c83dfb3a7d25e&to=f2f91e6a2743070471cc9471e4e8c646e50c653c&stat=instructions

There were no text-size changes this time.

[serge-sans-paille]

llvm/lib/IR/LegacyPassManager.cpp

1617–1619

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

[foad]

llvm/lib/IR/LegacyPassManager.cpp

1617–1619

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

[serge-sans-paille]

llvm/lib/IR/LegacyPassManager.cpp

1617–1619

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

llvm/lib/IR/LegacyPassManager.cpp

#endif
 
     Changed |= LocalChanged;
     if (LocalChanged)
       dumpPassInfo(FP, MODIFICATION_MSG, ON_FUNCTION_MSG, F.getName());
     dumpPreservedSet(FP);
     dumpUsedSet(FP);
 
     verifyPreservedAnalysis(FP);
+    if (LocalChanged)
       removeNotPreservedAnalysis(FP);
     recordAvailableAnalysis(FP);
     removeDeadPasses(FP, F.getName(), ON_FUNCTION_MSG);
   }
 
   return Changed;
 }
 
 bool FPPassManager::runOnModule(Module &M) {
   bool Changed = false;
#endif
     Changed |= LocalChanged;
     if (LocalChanged)
       dumpPassInfo(MP, MODIFICATION_MSG, ON_MODULE_MSG,
                    M.getModuleIdentifier());
     dumpPreservedSet(MP);
     dumpUsedSet(MP);
 
     verifyPreservedAnalysis(MP);
+    if (LocalChanged)
       removeNotPreservedAnalysis(MP);
     recordAvailableAnalysis(MP);
     removeDeadPasses(MP, M.getModuleIdentifier(), ON_MODULE_MSG);
   }
 
   // Finalize module passes
   for (int Index = getNumContainedPasses() - 1; Index >= 0; --Index)
     Changed |= getContainedPass(Index)->doFinalization(M);