Page MenuHomePhabricator

[NewPassManager] Add assertions when getting statefull cached analysis.
Needs RevisionPublic

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



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.Thu, Jan 30, 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.Fri, Feb 21, 1:00 AM
chandlerc added inline comments.

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


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.


Just sink the test into the if?


Same as above.


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.Fri, Feb 21, 1:00 AM
asbirlea marked an inline comment as done.Fri, Feb 21, 12:06 PM
asbirlea added inline comments.

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.Sat, Feb 22, 7:36 PM

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.