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

[Justin Lebar via Phabricator via llvm-commits]

jlebar accepted this revision.
jlebar 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) {
----------------
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.


================
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.
----------------
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.


================
Comment at: include/llvm/IR/PassManager.h:164
+
+  /// Mark a particular analysis ID as abandoned, removing it from the
+  /// preserved set even if covered by some other set.
----------------
chandlerc wrote:
> jlebar wrote:
> > Again here, we are still marking the analysis -- not an ID -- as abandoned.  The difference is in what we're *given*, not really what we *do*.
> Does clear terms (type vs. ID) help here? Or do you really want the verb moved around?
I'm fine with the "analysis ID" change; see above for the verb issue.


================
Comment at: include/llvm/IR/PassManager.h:560
+    ///
+    /// This is sadly less efficient than the above routine which leverages the
+    /// type parameter to avoid the type erasure overhead, but in some cases
----------------
chandlerc wrote:
> jlebar wrote:
> > ", which" and then start a new sentence at "but".
> > 
> > There is a rule for the comma here: If you have a "which/that/who" phrase that is not "narrowing", you almost always offset the phrase with commas.  A "narrowing" "which/that/who" phrase restricts the meaning of the phrase before:  "My coworker who uses ed is out sick." (No commas, I have more than one coworker.)  If the phrase is not narrowing, it gets a comma: "My dad, who is a programmer, lives in CA." (Commas; I have only one dad.)
> > 
> > Like everything in English, it depends, but this one is relatively safe.
> My problem is not that I'm not familiar with the rule about narrowing vs. non-narrowing, it is two-fold:
> 
> 1) When writing, I cannot keep both these rules and what I am trying to say in my head.
> 
> 2) When writing and reading my own text, I often end up with a very different interpretation of what it means to be a narrowing phrase. I think I read restrictions of meaning into things that English says aren't actually narrowing phrases.
> 
> Sorry. =/ I've been failing at this aspect of writing for over 15 years I fear.
Heh, okay.  Sorry that wasn't helpful.


================
Comment at: include/llvm/IR/PassManager.h:219
+    /// when querying the preserved sets in order to skip them, so the ID is
+    /// passed into the constructor to allow for a cached query.
+    PreservedAnalysisChecker(const PreservedAnalyses &PA, AnalysisKey *ID)
----------------
Kind of runs on with "in order to skip them".  I'd set it off in parens, but maybe it's just not necessary:

> Both \c preserved() and \c preservedSet() need to check whether the analysis was abandoned, so we take the analysis ID here and cache whether it was abandoned.

Or if you want to reveal fewer implementation details:

> A PreservedAnalysisChecker is tied to a particular Analysis because \c preserved() and \c preservedSet() both return false if the Analysis was abandoned.


================
Comment at: include/llvm/IR/PassManager.h:271
+  ///
+  /// This is only true when no analyses has been explicitly abandoned.
+  template <typename AnalysisSetT> bool allAnalysesInSetPreserved() const {
----------------
have been (or "no analysis has been" if you like)


================
Comment at: include/llvm/IR/PassManager.h:278
+  ///
+  /// This is only true when no analyses has been explicitly abandoned.
+  bool allAnalysesInSetPreserved(AnalysisSetKey *SetID) const {
----------------
ibid


================
Comment at: include/llvm/IR/PassManager.h:560
+    /// Helper to implement the invalidate methods above, see their
+    /// documentation for the detailed interface.
+    template <typename ResultT = ResultConceptT>
----------------
Now that I see this -- do we need this comment at all?  It's abundantly clear that this is a helper, and it's a private function.


https://reviews.llvm.org/D27198