[PATCH] D23738: [PM] Extend the explicit 'invalidate' method API on analysis results to accept an Invalidator that allows them to invalidate themselves if their dependencies are in turn invalidated.

[Justin Lebar via llvm-commits]

jlebar added a comment.

I am still trying to understand why we've designed it this way, but here are some comments on comments.



================
Comment at: include/llvm/IR/PassManager.h:365
+  /// erases. Provides both the pass ID and concept pointer such that it is
+  /// half of a bijection and provides storage for the actual result concept.
+  typedef std::list<std::pair<void *, std::unique_ptr<ResultConceptT>>>
----------------
I can't figure out what "half of a bijection" is, and this phrase apparently appears nowhere on the internet other than in this file.  Can we rephrase?


================
Comment at: include/llvm/IR/PassManager.h:365
+  /// erases. Provides both the pass ID and concept pointer such that it is
+  /// half of a bijection and provides storage for the actual result concept.
+  typedef std::list<std::pair<void *, std::unique_ptr<ResultConceptT>>>
----------------
jlebar wrote:
> I can't figure out what "half of a bijection" is, and this phrase apparently appears nowhere on the internet other than in this file.  Can we rephrase?
Comma before "and" -- new independent clause.


================
Comment at: include/llvm/IR/PassManager.h:369
+
+  /// \brief Map type from function pointer to our custom list type.
+  typedef DenseMap<IRUnitT *, AnalysisResultListT> AnalysisResultListMapT;
----------------
An IRUnitT is not necessarily a function pointer, nor a pointer to an llvm::Function, right?


================
Comment at: include/llvm/IR/PassManager.h:372
+
+  /// \brief Map type from a pair of analysis ID and function pointer to an
+  /// iterator into a particular result list.
----------------
same here wrt "function pointer"


================
Comment at: include/llvm/IR/PassManager.h:373
+  /// \brief Map type from a pair of analysis ID and function pointer to an
+  /// iterator into a particular result list.
+  typedef DenseMap<std::pair<void *, IRUnitT *>,
----------------
This comment in particular, but also the one above, just sort of spells out in English what the type is.  It's definitely helpful to tell me what "void*" is, but you could write that as

  std::pair</* AnalysisID */ void*, IRUnit *>

or even

  using AnalysisID = void*;
  std::pair<AnalysisID, IRUnit *>

Once we've made the code clearer in this way, perhaps we could use the comments to say something about what these types are for.


================
Comment at: include/llvm/IR/PassManager.h:379
 public:
-  // Most public APIs are inherited from the CRTP base class.
+  /// An API provided to analysis results when they are handling invalidation to
+  /// allow them to invalidate other analyses they depend on.
----------------
comma before "to"


================
Comment at: include/llvm/IR/PassManager.h:383
+  /// This is used to support analysis results which embed handles to other
+  /// analysis results and thus recursive invalidation must be performed. We
+  /// pass in this API from the analysis manager in order to let the analysis
----------------
Rephrase beginning at "and" -- bad parallelism.


================
Comment at: include/llvm/IR/PassManager.h:384
+  /// analysis results and thus recursive invalidation must be performed. We
+  /// pass in this API from the analysis manager in order to let the analysis
+  /// results themselves define the dependency graph rather than tracking and
----------------
Nit, we don't really pass the API per se.  "We pass in an Invalidator", perhaps?


================
Comment at: include/llvm/IR/PassManager.h:386
+  /// results themselves define the dependency graph rather than tracking and
+  /// hard coding it anywhere.
+  class Invalidator {
----------------
Rephrase starting with "rather than".  I think this is important, because it's kind of the motivation for the whole design, but I don't grok what you're trying to get across.


================
Comment at: include/llvm/IR/PassManager.h:401
+
+      // Otherwise lookup the result object.
+      auto RI = Results.find({PassID, &IR});
----------------
"look up" is still the verb form.  I am losing this fight, but I have at least a few more years before the space is gone forever.  (The space makes the written form of the verb match the spoken form, which has two accented syllables -- the spoken noun form has only one accented syllable -- "LOOKup TAble", versus "LOOK UP 'TAble' in the dictionary".)


================
Comment at: include/llvm/IR/PassManager.h:589
+    // to an Invalidator instance to additionally memoize the calls to each
+    // result's invalidate routine.
+    SmallDenseMap<void *, bool, 8> IsResultInvalidated;
----------------
Kind of runs on -- split into two sentences?

  Track whether each pass's result is invalidated.  Memoize the results using the IsResultInvalidated map.


================
Comment at: include/llvm/IR/PassManager.h:595
+      // We don't directly use the Invalidator here because we have to operate
+      // on the type erased result where it doesn't, but we don't have to look
+      // up the result in the map where it does.
----------------
type-erased


================
Comment at: include/llvm/IR/PassManager.h:595
+      // We don't directly use the Invalidator here because we have to operate
+      // on the type erased result where it doesn't, but we don't have to look
+      // up the result in the map where it does.
----------------
jlebar wrote:
> type-erased
s/where/whereas/


================
Comment at: include/llvm/IR/PassManager.h:595
+      // We don't directly use the Invalidator here because we have to operate
+      // on the type erased result where it doesn't, but we don't have to look
+      // up the result in the map where it does.
----------------
jlebar wrote:
> jlebar wrote:
> > type-erased
> s/where/whereas/
s/but/and


================
Comment at: include/llvm/IR/PassManager.h:596
+      // on the type erased result where it doesn't, but we don't have to look
+      // up the result in the map where it does.
+      void *PassID = AnalysisResultPair.first;
----------------
again s/where/whereas/ (or "while") would help, I think.

Or maybe just

  This is basically the same thing as Invalidator::invalidate, but we can't call it here because we're operating on the type-erased result.  Moreover if we instead called invalidate() directly, it would do an unnecessary lookup in ResultsList.

Or maybe even leave out that second sentence, because it's a premature optimization.  :)


================
Comment at: include/llvm/IR/PassManager.h:604
+      if (!Inserted)
+        // This result was already handled via the invalidator.
+        continue;
----------------
s/invalidator/Invalidator/?


================
Comment at: include/llvm/IR/PassManager.h:613
+    // Now re-walk the list of results but this time erasing the ones which
+    // have been invalidated.
+    for (auto I = ResultsList.begin(), E = ResultsList.end(); I != E;) {
----------------
No need to set up a contrast, I think.  Maybe simply

  Now erase the results that were invalidated.


================
Comment at: include/llvm/IR/PassManager.h:626
+      I = ResultsList.erase(I);
+      AnalysisResults.erase({PassID, &IR});
     }
----------------
(I don't know why we have both a list and a map when we have things like MapVector, but...okay, let's assume there's a good reason for this.  :)


================
Comment at: include/llvm/IR/PassManagerInternal.h:99
   ///
   /// \returns true if the result is indeed invalid (the default).
+  virtual bool invalidate(IRUnitT &IR, const PreservedAnalysesT &PA,
----------------
Based on this comment I have no idea what I am supposed to do with the InvalidatorT object.


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:76
 
+  // Invaliadtion was handled after each pass in the above loop for the current
+  // unit of IR. Therefore, the remaining analysis results in the
----------------
Invaliadtion

(I wonder how hard it would be to make a good C++ spellchecker.  You could imagine a tool that spellchecks only comments, and doesn't flag tokens that appear elsewhere in the code (outside comments)...  Catching these typos doesn't seem like a good job for hoomans.)

((Now I'm thinking about using whatever tech Chrome's online spellchecker -- which is hauntingly accurate -- uses, not just to catch typos in comments, but also bugs in code...))


================
Comment at: lib/Analysis/CGSCCPassManager.cpp:79
+  // AnalysisManager are preserved. We mark this with a set so that we don't
+  // need to inspect each one individually.
+  PA.preserve<AllAnalysesOn<LazyCallGraph::SCC>>();
----------------
I don't understand this comment, particularly "with a set".


================
Comment at: unittests/IR/PassManagerTest.cpp:392
+
+/// A test analysis pass which caches in its result another analyses pass and
+/// uses it to serve queries, and must therefore invalidate itself when its
----------------
s/analyses/analysis/


================
Comment at: unittests/IR/PassManagerTest.cpp:393
+/// A test analysis pass which caches in its result another analyses pass and
+/// uses it to serve queries, and must therefore invalidate itself when its
+/// dependency is invalidated.
----------------
Kind of a run-on sentence, maybe split?


================
Comment at: unittests/IR/PassManagerTest.cpp:426
+struct LambdaPass : public PassInfoMixin<LambdaPass> {
+  template <typename T> LambdaPass(T &&Arg) : Func(std::forward<T>(Arg)) {}
+
----------------
Can this constructor just take an std::function instead?  That would mean you have to spell out the type of the std::function twice, but I think it would be more explicit.


================
Comment at: unittests/IR/PassManagerTest.cpp:450
+  // First just use the analysis to get the instruction count, and preserve
+  // everything
+  FPM.addPass(LambdaPass([&](Function &F, FunctionAnalysisManager &AM) {
----------------
Nit, missing period.


================
Comment at: unittests/IR/PassManagerTest.cpp:458
+  // another function invalidate the indirect analysis, and for the final
+  // function invalidate both.
+  FPM.addPass(LambdaPass([&](Function &F, FunctionAnalysisManager &AM) {
----------------
The "for one function invalidate" form is awkward for me.  Maybe this would be easier to read as a bulleted list:

  Next, invalidate
    - the indirect analysis for g(),
    - the underlying analysis for h(), and
    - both analyses for f().

It might also make sense to rejigger the code so that we can change the comment to read "f()", "g()", and "h()".


================
Comment at: unittests/IR/PassManagerTest.cpp:469
+  }));
+  // Finally, use the analysis again on each function forcing re-computation
+  // for all of them.
----------------
comma before "forcing"


================
Comment at: unittests/IR/PassManagerTest.cpp:479
+
+  // There are generally two possible runs for ecah of the three functions. But
+  // for one function, we only invalidate the indirect analysis so the base one
----------------
ecah


================
Comment at: unittests/IR/PassManagerTest.cpp:483
+  EXPECT_EQ(5, AnalysisRuns);
+  // The indiract analysis is invalidate for each function (either directly or
+  // indirectly) and run twice total for each.
----------------
sp, indiract

is invalidated


================
Comment at: unittests/IR/PassManagerTest.cpp:484
+  // The indiract analysis is invalidate for each function (either directly or
+  // indirectly) and run twice total for each.
+  EXPECT_EQ(6, IndirectAnalysisRuns);
----------------
Not sure "total" is helpful.


================
Comment at: unittests/IR/PassManagerTest.cpp:488
+  // There are 5 total instructions in the module and we add the count three
+  // times total.
+  EXPECT_EQ(5 * 3, InstrCount);
----------------
Again not sure the second "total" helps.


https://reviews.llvm.org/D23738