Page MenuHomePhabricator

[NewPassManager] Add assertions when getting statefull cached analysis.
ClosedPublic

Authored by asbirlea on Jan 16 2020, 4:58 PM.

Details

Summary

Analyses that are statefull should not be retrieved through a proxy from
an outer IR unit, as these analyses are only invalidated at the end of
the inner IR unit manager.
This patch disallows getting the outer manager and provides an API to
get a cached analysis through the proxy. If the analysis is not
stateless, the call to getCachedResult will assert.

Diff Detail

Event Timeline

asbirlea created this revision.Jan 16 2020, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2020, 4:58 PM
asbirlea updated this revision to Diff 238920.Jan 17 2020, 4:03 PM

Minor updates.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

asbirlea updated this revision to Diff 241498.Jan 30 2020, 10:03 AM

Rebase and gentle ping.

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

chandlerc requested changes to this revision.Feb 21 2020, 1:00 AM
chandlerc added inline comments.
llvm/include/llvm/IR/PassManager.h
800

Could this logic be moved into the callers that pass true here to avoid the need for a book parameter?

llvm/unittests/Analysis/CGSCCPassManagerTest.cpp
321

You added a book returning method for unittesting without triggering the assert, is that not enough here? It does seem much cleaner to me than the friend declaration - much harder to misuse.

395

Just sink the test into the if?

412

Same as above.

893

Here and below where similar patterns actually necessitate the friend function, I think the test is actually exercising specific functionality that your change makes unsupported, so these tests should be changed or removed.

I think they still are useful for the case where the outer analysis would survive a preserve-none invalidate, but *not* survive *forced* invalidation. In that case the cached results should be fine and safe (and your assert pass) but still require this test to register the outer analysis dependency to handle forced invalidation.

I think you can update this to use your new API making the cached result return false on a call to invalidate, but then forcing it's invalidation where necessary.

Some tests may become no-ops or redundant with that change and those should just be removed. Does that make sense?

This revision now requires changes to proceed.Feb 21 2020, 1:00 AM
asbirlea marked an inline comment as done.Feb 21 2020, 12:06 PM
asbirlea added inline comments.
llvm/include/llvm/IR/PassManager.h
1147

A quick clarification before addressing all the comments: only FunctionAnalysisManagerCGSCCProxy is a friend and can get the actual manager by calling getInnerManager.
This is used in lib/Analysis/CGSCCPassManager.cpp:226 (I need to update the naming below, it's not MAMProxy, it's MAM).

The unit tests do not have access to this API. They are using managers already declared in the test itself and it's probably worth not allowing even that, because, you know, it's hack to allow something disallowed...
As you mentioned, I'll look to delete or update the tests wherever the API cachedResultExists cannot be used.

chandlerc added inline comments.Feb 22 2020, 7:36 PM
llvm/lib/Analysis/CGSCCPassManager.cpp
235–241

So, now that I actually read this code correctly, I understand better why this just doesn't fit anything.

The problem here is that we're not supposed to reconstruct module analyses that *must* be available at this layer. Those are supposed to be passed as parameters to the run method the way the LazyCallGraph is. Because then, an actual module pass (the CG adapter) will run them (as it can) and pass them along. But that's not a good fit *here* because every other CGSCC pass should use *this* proxy rather than one passed in...

The CGSCC layering is so complicated. I really wish I had any ideas how to really simplify things. Anyways.

I think there is one step that would be slightly cleaner than this. I would change this to just assert that the FAM proxy has been cached, and to construct an *empty* result object.

Then I would change the result object to allow the CGSCC walk (CGSCCPassManager.h:833) to actually get the result, and update it with the FAM. There, the code is actually a *module* pass, and so it can directly get the FAMProxy without issue. That will also resolve the FIXME on line 831 in that file as there will be a clear reason why we have to specially set up this proxy -- we need to pass something from the Module layer into the SCC layer that could otherwise be invalidated.

asbirlea updated this revision to Diff 251493.Mar 19 2020, 3:59 PM
asbirlea marked 2 inline comments as done.

Address a comment.

asbirlea added inline comments.Mar 19 2020, 3:59 PM
llvm/lib/Analysis/CGSCCPassManager.cpp
235–241

I made the changes according to this suggestion.
Some notable changes:

  • The empty result needs updating whenever it was retrieved before to get it created, and after an invalidate event.
  • Updated updateCGAndAnalysisManagerForFunctionPass and updateCGAndAnalysisManagerForCGSCCPass APIs to need a FunctionaAnalysisManager. This affects Coroutines, CallGraphUpdater (needed by Attributor), Inliner and the unit tests in CGSCCPassManagerTest.
  • I left a comment at ~CGSCCPassManager.cpp:235. Having the assert to check that the FunctionAnalysisManagerModuleProxy exists will change the pass order in the tests, and it will behave differently with vs without asserts. So this would either be removed entirely (the tests are updated in this diff), or have the cachedResultExists run outside of the assert and (void) the result in release mode (the tests won't change then).

Please let me know what you think.

asbirlea updated this revision to Diff 251503.Mar 19 2020, 4:46 PM
asbirlea marked 4 inline comments as done.

Update unit test.

asbirlea updated this revision to Diff 253012.Mar 26 2020, 4:29 PM

Simplified the instances where the proxy result needs to set the FAM.
Added comments on where this is needed (when an SCC is first seen not after an invalidation).

asbirlea marked 2 inline comments as done.Mar 26 2020, 4:35 PM
asbirlea added inline comments.
llvm/include/llvm/IR/PassManager.h
800

I haven't addressed this. Yes, I can move the logic in to the caller but I need to make the AnalysisManagerT::Invalidator constructor public.
Is there another way to do this?

llvm/lib/Analysis/CGSCCPassManager.cpp
235–241

Re-added the assert and undid the test changes. This always gets the result for the proxy here, as before.

asbirlea updated this revision to Diff 254378.Apr 1 2020, 5:40 PM

Update clang test.

Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 5:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Really like the approach now. Pretty minor code nits below only. =D

llvm/include/llvm/Analysis/CGSCCPassManager.h
844–850

Check that it doesn't hit an assert failure, but I think you can remove this one.

llvm/include/llvm/IR/PassManager.h
800

Ah, good point.

How about making this a verifyNotInvalidated method separate from getCachedResult? That still seems a bit cleaner to me than a bool parameter. What do you think?

llvm/lib/Analysis/CGSCCPassManager.cpp
247–249

Don't fully understand this comments wording... Maybe something more along the lines of:

We just return an empty result. The caller will use the updateFAM interface to correctly register the relevant FunctionAnalysisManager based on the context in which this proxy is run.

340

Omit the variable?

403–408

Can this be simplified to:

FunctionAnalysisManager *FAM = nullptr;
if (auto *FAMProxy = AM.getCachedResult<FunctionAnalysisManagerCGSCCProxy(*OldC))
  FAM = &FAMProxy->getManager();

And then use FAM being non-null below instead of the NeedFAMProxy bool?

702

Could omit the variable?

llvm/lib/Transforms/IPO/Inliner.cpp
960–965

Add a blank line between the paragraphs?

asbirlea updated this revision to Diff 259450.Apr 22 2020, 7:07 PM
asbirlea marked 8 inline comments as done.

Address comments.

asbirlea added inline comments.Apr 22 2020, 7:09 PM
llvm/include/llvm/Analysis/CGSCCPassManager.h
844–850

Removing this hits the assertion.

Gentle ping for more comments :-)

Weekly re-ping.

chandlerc accepted this revision.May 12 2020, 4:37 PM

LGTM other than two nits here, this is really awesome!

llvm/include/llvm/Analysis/CGSCCPassManager.h
844–850

Dunno what I was thinking. Of course it does.

Anyways, skip the variable and just update the result directly?

907

Similar to above, skip the variable?

This revision is now accepted and ready to land.May 12 2020, 4:37 PM
asbirlea updated this revision to Diff 263590.May 12 2020, 6:53 PM
asbirlea marked 3 inline comments as done.

Address comments.
Thank you for the review!

This revision was automatically updated to reflect the committed changes.