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

Fri Dec 16 01:51:12 PST 2016

chandlerc added a comment.

Patch updated with fixes, see responses below.

================
Comment at: include/llvm/IR/PassManager.h:675
   void invalidate(IRUnitT &IR, const PreservedAnalyses &PA) {
-    // Short circuit for common cases of all analyses being preserved.
-    if (PA.areAllPreserved() || PA.preserved<AllAnalysesOn<IRUnitT>>())
+    // We can only use one short circuit because even if another set is
+    // preserved there might be abandoned analyses within that set which still
----------------
jlebar wrote:
> chandlerc wrote:
> > jlebar wrote:
> > > It's not clear what this is contrasting with (without the diff available :).
> > Reworded to hopefully make this more clear.
> With your latest change to the PA interface, can we revert this change and instead do
> 
>   if (PA.areAllPreserved() || PA.allAnalysesOnSetPreserved<AllAnalysesOn<IRUnitT>>())
> 
> ?
We can. It will never fire though because we instead do this test before walking all the IR units so that if we have 10million functions we don't query the PA 10 million times for this...

But we already query it for the all preserved.... so it seems silly in retrospect.

And what's more, the all query should really be handled in the allAnalysesOnSetPreserved, making this quite nice now.

We can keep the optimization in the callers that do this in a tight loop.

================
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:
> 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"?

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

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

================
Comment at: include/llvm/IR/PassManager.h:227
+  /// We need to know whether the analysis has been explicitly abandoned even
+  /// when querying the preserved sets in order to skip them.
+  class PreservedAnalysisChecker {
----------------
jlebar wrote:
> Perhaps this sentence should live on the constructor:
> 
> > We take an AnalysisKey in our constructor because we need to know ...
> 
> I think maybe you had this comment here because you wanted to clarify what `preserved` and `preservedSet` return without writing repetitive comments?  I think it's probably worth having brief comments there:
> 
> > /// Returns true if our analysis was not abandoned and (a) the analysis was explicitly preserved, or (b) all analyses were preserved.
> 
> > /// Returns true if our analysis was not abandoned and (a) the set was explicitly preserved, or (b) all analyses were preserved.
Done but with slightly different wording. See what you think.

================
Comment at: include/llvm/IR/PassManager.h:297

-  SmallPtrSet<AnalysisKey *, 2> PreservedAnalysisIDs;
+  /// The set of preserved analyses.
+  ///
----------------
jlebar wrote:
> Suggest
> 
> > The analyses and analysis sets that are preserved.
> >
> > Invariant: A given AnalysisKey is never in both PreservedIDs set and NotPreservedAnalysisIDs.
Took a slightly different approach but same spirit.

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

================
Comment at: lib/Analysis/LoopPassManager.cpp:37
+  auto PAC = PA.getChecker<LoopAnalysisManagerFunctionProxy>();
+  if (!PAC.preserved())
     InnerAM->clear();
----------------
jlebar wrote:
> You don't want to check an AllAnalysesOn here too?
I can, I just wasn't bothering because this isn't really tested or updated at all yet. I just made it compile and behave exactly as it did before.

https://reviews.llvm.org/D27198