[PATCH] D100912: [docs][NewPM] Add section on analyses

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/docs/NewPassManager.rst:147
+LLVM provides many analyses that passes can use, such as a dominator tree.
+Calculating these can be expensive, so the new pass manager has
+infrastructure to cache analyses and reuse them when possible.
----------------
So does the LPM. We could say "similar to the legacy pass manager, the new pass manager has...". I'd add "there are differences, though, in how passes communicate to the infrastructure to ensure the validity of the cached values"


================
Comment at: llvm/docs/NewPassManager.rst:199
+There are two ways to deal with potentially invalid analysis results. One is
+to simply force clear the results. This should generally only be used when
+the IR that the result is keyed on becomes invalid. For example, a function
----------------
I'd stress here that this must be used when deleting the IR, explicitly, by the pass - and show the code example. I'd also stress that if passes don't do that, the risk is that the same key (==obj pointer) may be used for newly allocated IR and the stale cached value may appear valid when it's not.


================
Comment at: llvm/docs/NewPassManager.rst:239
+
+An analysis should implement ``invalidate()`` like below, at least for simple
+cases:
----------------
but for simple cases that's not necessary, the default impl does that, no?


================
Comment at: llvm/docs/NewPassManager.rst:293
+for more details.
+
 Status of the New and Legacy Pass Managers
----------------
I'd recommend adding a subsection with guidelines for analysis authors, and one for consumers of analyses (i.e. pass authors), distilling the design into actionable DO/DON'T statements. Like "don't implement invalidate() and return false, unless you really mean to develop an immutable analysis*)

*assuming we fix the issues of bulk-invalidation by the likes of cgscc AM, but I digress :)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100912/new/

https://reviews.llvm.org/D100912