[PATCH] D27198: [PM] Introduce the facilities for registering cross-IR-unit dependencies that require deferred invalidation.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Ok, two significant changes here:

1. I've made the sets use a distinct key type from the analyses so they are clearly independent things. I've also separated the APIs dealing with them so that things are more explicit.

2. Justin and I sat down together to try to at least remove the "magical" aspect of the query API on `PreservedAnalyses`. The result is a very different API. It is a bit more heavyweight, but now the set relationships can be explicit logical relation ships of ||s and &&s rather than a list of things. This seems both more expressive and also more readable. It seemed to make the code implementing the `invalidate` method somewhat more comprehensible to Justin at least. But I'd like general feedback on this API design.

As Sean had suggested (thanks!) and then amplified by #2, I've rewritten the high level documentation for `PreservedAnalyses` and I've written much more comprehensive unit testing. It now clearly needs to live in a separate file IMO, but I want to do that code movement as a follow-up patch if that's OK.

I also spent some time trying to understand why elements of this are confusing, especially at first. I think Justin had a great insight here that the names of the critical component--the proxy analyses--give the reader no good anchor to what each one *is*. Once that is established, understanding the code becomes much easier.

The current plan is to rename the proxies from 'FunctionAnalysisManagerModuleProxy' to 'ProxyModuleAnalysis<FunctionAnalysisManager>'. This does two things that seem to help. One is that it puts 'ModuleAnalysis' early and contiguous in the name anchoring the reader that this is a module analysis first and foremost. The second is that it sinks the thing being proxied into a clearly subordinate position. This too seems like it should be its own patch.

A final thought is that there is currently some duplication of code between the two ProxyModuleAnalysis results' invalidate implementation that it turns out I *can* nicely factor out. I'm happy to do that in this patch or a follow-up, whatever folks prefer. I just wanted to update the patch now that the API rework is in place.

Some further replies below...



================
Comment at: include/llvm/IR/PassManager.h:75
 /// the IR is not mutated at all.
 class PreservedAnalyses {
 public:
----------------
silvas wrote:
> The comment on this class needs to be beefed up in response to this patch. The original idea behind PreservedAnalyses was that it was a whitelist of analyses to preserve, so that invalidation was conservatively correct. The addition of `NotPreservedAnalysisIDs` breaks with this approach and adds a layer of complexity that isn't adequately addressed by the current comment.
I've written new documentation that tries to do this. It may not be good or enough. Let me know how this does and what else I can do here.


================
Comment at: include/llvm/IR/PassManager.h:109
+  ///
+  /// Note that you can only abandon a specific analysis, not a *set* of
+  /// analyses.
----------------
silvas wrote:
> This restriction seems like it stems from an implementation detail. Some implementation-level comment should explain its origin.
It actually is more of an interface and semantic simplification in my mind... What does it mean to abandon a set? Does that abandon even explicitly preserved analyses? I would assume so (that's how analysis abandonment works), but instead we might just remove the set. Spelling that out will be necessary.

Also, supporting abandonment of sets makes the API for querying even more constrained, and the complexity of the interface was one thing that was raised as an unfortunate complexity in this patch.

With the new `PreservedAnalyses` API the code is simpler but we now can't express abandoned analysis sets reasonably at the interface level if they actually preclude individual analysis preservation.

I'm not sure what documentation would help here though... thoughts?


================
Comment at: include/llvm/IR/PassManager.h:244
+  /// analysis not preserved. It always wins over the above set and should not
+  /// include any synthetic set IDs such as the "all" ID.
+  SmallPtrSet<AnalysisKey *, 2> NotPreservedAnalysisIDs;
----------------
silvas wrote:
> This restriction about no sets in `NotPreservedAnalysisIDs` needs to be explained better in the comments here. More generally, there is a clear asymmetry between the `PreservedAnalysisIDs` and `NotPreservedAnalysisIDs` that needs to be explained (not just the "rules", but *why* the rules are there). Maybe that is appropriate for the class comment?
See above... in some ways the asymmetry is worse (we have it at the typesystem level). But I'm not sure how best to document this. Suggestions would really be helpful here. The why is both "we don't need it, so why add it?" coupled with the fact that adding support for it introduces non-trivial complexity to the API.

Like I said, I'm very happy to add documentation that you think would help in light of the new API, just need to know what and where.


https://reviews.llvm.org/D27198