Page MenuHomePhabricator

[NewPM][LVI] Abandon LVI after CVP
ClosedPublic

Authored by nikic on Thu, Jul 30, 11:01 AM.

Details

Summary

As mentioned on D70376, LVI can currently cause performance issues when running under NewPM. The root problem is that, unlike the legacy pass manager, NewPM will not immediately discard the LVI analysis if the following pass does not need it. This is a problem, because LVI has a high memory requirement, and mass invalidation of LVI values is very inefficient. LVI should only be alive during passes that actively interact with it.

This patch addresses the issue by explicitly abandoning LVI after CVP, which gets us back to the LegacyPM behavior.

I don't think something like this is done anywhere else, so I'm wondering whether this kind of explicit abandonment of analysis passes under NewPM is acceptable.

Diff Detail

Event Timeline

nikic created this revision.Thu, Jul 30, 11:01 AM
nikic requested review of this revision.Thu, Jul 30, 11:01 AM
aeubanks added inline comments.Thu, Jul 30, 11:04 AM
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
89

can we skip this option and update the pipeline tests?

Thanks for the fix! I tend to agree with @aeubanks comment that it might be better to make this unconditional and change the tests (and the comment "Any invalidation that shows up here is a bug" that shows up at the top of at least one of the pipeline tests - it should probably just say something like "Any invalidation that shows up in a non-abandoned analysis is a bug".

Here is how updating one of the tests would look like: https://gist.github.com/nikic/577dead281f1aee10b492c3fc7130f5b

This would look nice if it only printed when an analysis actually gets invalidated, but I feel that the large number of "Invalidating all non-preserved analyses for" messages make this test worse.

tejohnson accepted this revision.Thu, Jul 30, 12:31 PM

Here is how updating one of the tests would look like: https://gist.github.com/nikic/577dead281f1aee10b492c3fc7130f5b

This would look nice if it only printed when an analysis actually gets invalidated, but I feel that the large number of "Invalidating all non-preserved analyses for" messages make this test worse.

Agree that is ugly. I'm ok with using the option then. The patch LGTM (but looks like it needs a clang format). Please wait to see if @aeubanks has any more comments.

This revision is now accepted and ready to land.Thu, Jul 30, 12:31 PM

Hmm I wonder if we can just delete the "Invalidating all non-preserved analyses for" log statement, since "Invalidating analysis: FooAnalysis" does get printed when an analysis is actually invalidated. Or maybe at least only print it only when any analyses are actually invalidated (as you said). I'll take a quick look.

Also, looks like thinlto-distributed-newpm.ll is failing.

Hmm I wonder if we can just delete the "Invalidating all non-preserved analyses for" log statement, since "Invalidating analysis: FooAnalysis" does get printed when an analysis is actually invalidated. Or maybe at least only print it only when any analyses are actually invalidated (as you said). I'll take a quick look.

Also, looks like thinlto-distributed-newpm.ll is failing.

Sorry, I think I misunderstood the issue. LazyValueAnalysis is actually being invalidated, right? So "Invalidating all non-preserved analyses for" would get printed regardless if we went with the option of only printing it when an analysis is actually invalidated.
What about removing all "Invalidating all non-preserved analyses for" and checking that there's no "Invalidating analysis: " aside from LazyValueAnalysis? That's probably worse than this solution...

The additional debug print with "Invalidating all non-preserved analyses for" seems too noisy and not informative.
+1 on rebasing this without the no-lvi-abandon flag on D84981.

The additional debug print with "Invalidating all non-preserved analyses for" seems too noisy and not informative.
+1 on rebasing this without the no-lvi-abandon flag on D84981.

It's been submitted now.

nikic updated this revision to Diff 282254.Fri, Jul 31, 10:06 AM
nikic edited the summary of this revision. (Show Details)

Drop -lvi-no-abandon flag, update pipeline tests.

nikic added inline comments.Fri, Jul 31, 10:07 AM
llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
233

The test diffs looks reasonable to me now, apart from this part. Is this expected?

aeubanks added inline comments.Fri, Jul 31, 12:02 PM
llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
233

That is weird, I'll take a look.

tejohnson added inline comments.Fri, Jul 31, 12:10 PM
llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
233

I was just looking at this. The message "Clearing all analysis results for: <possibly invalidated loop>" comes from the callsite to AnalysisManager<IRUnitT, ExtraArgTs...>::clear from LoopAnalysisManagerFunctionProxy::Result::invalidate. Looking at the conditions for that call, I'm guessing that the following is no longer true:
PAC.preservedSet<AllAnalysesOn<Function>>()
since the abandon() call will remove that ID from the preserved set.

So it is probably "expected" by the code change here. Not sure if there is a way to avoid it, i.e. is that code overly conservative.

I had a random thought, maybe CVP itself shouldn't be invalidating LVI, but rather it should be explicitly invalidated in PassBuilder::buildFunctionSimplificationPipeline() with a InvalidateAnalysisPass<LazyValueAnalysis>. Since the description mentions that this is based on the knowledge that no passes after CVP will use LVI. ???

llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
233

DominatorTree::invalid() checks PAC.preserved() || PAC.preservedSet<AllAnalysesOn<Function>>() || PAC.preservedSet<CFGAnalyses>(). Even though PAC.preservedSet<AllAnalysesOn<Function>>() will be false, PAC.preserved() should still be true.

aeubanks added inline comments.Fri, Jul 31, 12:32 PM
llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
233

Oh sorry, now I see what you mean, yes this is expected.
I don't follow the reasoning in the comments, but that's a different issue :)

aeubanks accepted this revision.Fri, Jul 31, 12:51 PM

I had a random thought, maybe CVP itself shouldn't be invalidating LVI, but rather it should be explicitly invalidated in PassBuilder::buildFunctionSimplificationPipeline() with a InvalidateAnalysisPass<LazyValueAnalysis>. Since the description mentions that this is based on the knowledge that no passes after CVP will use LVI. ???

I'm not super familiar with the details, but if LVI should really always created on demand then immediately invalidated after CVP, then this lgtm.

I had a random thought, maybe CVP itself shouldn't be invalidating LVI, but rather it should be explicitly invalidated in PassBuilder::buildFunctionSimplificationPipeline() with a InvalidateAnalysisPass<LazyValueAnalysis>. Since the description mentions that this is based on the knowledge that no passes after CVP will use LVI. ???

I'm not super familiar with the details, but if LVI should really always created on demand then immediately invalidated after CVP, then this lgtm.

Yeah I was pondering the same thing (since it looks like CVP is invoked twice per function), but presumably intermediate passes may hit the update compile time issue described in D70376. So I think for now this is a good approach to address the regression?

nikic added a comment.Sat, Aug 1, 12:55 AM

I don't really know what the tradeoff would be between using abandon() here and using InvalidateAnalysisPass. I went with this variant as @asbirlea indicated in https://reviews.llvm.org/D70376#2151283 that this would be preferred, as InvalidateAnalysisPass is really more intended for testing purposes.

tejohnson accepted this revision.Sat, Aug 1, 9:30 AM

I don't really know what the tradeoff would be between using abandon() here and using InvalidateAnalysisPass. I went with this variant as @asbirlea indicated in https://reviews.llvm.org/D70376#2151283 that this would be preferred, as InvalidateAnalysisPass is really more intended for testing purposes.

Yep, agree this is a good approach for now at least. Since the require is in CVP, logically it makes sense to abandon it there too. The only advantage of doing it in the pass pipeline setup would be if it made sense to abandoning it once after both CVP invocations, but that might not avoid the memory and compile time issues.

This is ok to check in to resolve the regression.
I'll go back and check if that invalidate is overly conservative, but don't block in it.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSat, Aug 1, 2:52 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript