[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 inline comments.


================
Comment at: include/llvm/IR/PassManager.h:133
 
-  /// \brief Mark an abstract ID as preserved, adding it to the set.
+  /// Mark an abstract analysis ID as preserved.
   void preserve(AnalysisKey *ID) {
----------------
jlebar wrote:
> chandlerc wrote:
> > jlebar wrote:
> > > Maybe
> > > 
> > > > Mark a particular analysis as preserved, given a pointer to its AnalysisKey.
> > > 
> > > or something.  The current way of distinguishing between this and the one above -- "a particular analysis" versus "an abstract analysis ID" -- is not facile.
> > > 
> > > Same below.
> > How about "an analysis type" vs. "an analysis ID"?
> If all I had were the comments and function signatures, I think I might still find this confusing -- is marking the analysis ID as preserved somehow different than marking a type as preserved?  Given the implementation of the first function, it's pretty clear to me, though, so I think it's probably fine if you want to do it this way.
Ah, I think I see a better way of wording this. Try now?


================
Comment at: include/llvm/IR/PassManager.h:157
+  /// Mark a particular analysis as abandoned, removing it from the preserved
+  /// set even if covered by some other set or previously explicitly marked as
+  /// preserved.
----------------
jlebar wrote:
> chandlerc wrote:
> > jlebar wrote:
> > > "if it's covered" "was previously marked as preserved".
> > No, the *set* isn't even temporal. An abandoned analysis is subtracted from all sets at query time.
> > Mark an analysis type as abandoned, removing it from the preserved set even if covered by some other set or previously explicitly marked as preserved.
> 
> vs.
> 
> > Mark an analysis type as abandoned, removing it from the preserved set even if it's covered by some other set or was previously explicitly marked as preserved.
> 
> To me, these two sentences have no semantic difference -- the first one is just eliding some verbs.  It sounds like these two mean something different to you?
> 
> Based on these comments, I think maybe you want to get across that abandoning an analysis undoes explicit preservation, but that "implicit" preservation via a set does not undo abandonment.  If that's it, how about:
> 
> > Mark an analysis type as abandoned.  An abandoned analysis is not part of the preserved set, even if it is nominally covered by some other set or was previously explicitly marked as preserved.
> 
> possibly s/is not part/will not be part/
> possibly s/is not part/is not considered part/
> 
> If you think that's still not clear, maybe we just need an example; that would make it unambiguous.  This a tricky but important API.
I like your second wording attempt. That really gets at the heart of it I think. I'm happy to add an example if it helps though.


https://reviews.llvm.org/D27198