[PATCH] D59315: [AliasAnalysis] Second prototype to cache BasicAA / anyAA state.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

FWIW, I really like the direction and design here. Thanks so much for pushing all of this through, I think it is a really nice generalization of the caching done by BasicAA that lets us do more and more general things of this nature.

There are several places where it seemed like `clear` should be called and wasn't. I marked all of the ones I saw just to avoid missing it in future iterations. But all of that may be obviated by my comment about the member query info object.



================
Comment at: include/llvm/Analysis/AliasAnalysis.h:289
 
+class AAQueryInfo {
+private:
----------------
Need comments for the class.


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:295-299
+  using AliasCacheTy = SmallDenseMap<LocPair, AliasResult, 8>;
+  AliasCacheTy AliasCache;
+
+  using IsCapturedCacheTy = SmallDenseMap<const Value *, bool, 8>;
+  IsCapturedCacheTy IsCapturedCache;
----------------
nitpick that should probably be a future cleanup and not part of this patch: I'd love to move these to use `FooT` instead of `FooTy`. The former is more commonly used for normal type aliases while the latter is more commonly referencing an LLVM `Type`.

My assumption is that these need to retain the spelling used by basic-aa to avoid unrelated churn, so happy for this to happen separately / later.


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:305-306
+
+  void clear() {
+    if (ClearCaches) {
+      AliasCache.shrink_and_clear();
----------------
Do we actually need this boolean? Is this just for the case of AST and this goes away in the future?


================
Comment at: include/llvm/Analysis/AliasAnalysis.h:770-771
   std::vector<AnalysisKey *> AADeps;
+
+  AAQueryInfo AAQIP;
+
----------------
You're using `shrink_and_clear` above to clear this between queries. As a consequence, it isn't clear to me why this being a member is still important. Maybe I missed it in the code using this, if so just point it out and sorry for the noise.

But it seems like you might be able to create this on the stack in overload of `alias` that doesn't accept it as a parameter?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:149
 ModRefInfo AAResults::getModRefInfo(Instruction *I, const CallBase *Call2) {
+  return getModRefInfo(I, Call2, AAQIP);
+}
----------------
No clear?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:434
                                     const MemoryLocation &Loc) {
+  return getModRefInfo(L, Loc, AAQIP);
+}
----------------
no clear?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:458
                                     const MemoryLocation &Loc) {
+  return getModRefInfo(S, Loc, AAQIP);
+}
----------------
No clear?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:489
 ModRefInfo AAResults::getModRefInfo(const FenceInst *S, const MemoryLocation &Loc) {
+  return getModRefInfo(S, Loc, AAQIP);
+}
----------------
No clear?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:504
                                     const MemoryLocation &Loc) {
+  return getModRefInfo(V, Loc, AAQIP);
+}
----------------
No clear?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:533
                                     const MemoryLocation &Loc) {
+  return getModRefInfo(CatchPad, Loc, AAQIP);
+}
----------------
No clear?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:552
                                     const MemoryLocation &Loc) {
+  return getModRefInfo(CatchRet, Loc, AAQIP);
+}
----------------
No clear?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:571
                                     const MemoryLocation &Loc) {
+  return getModRefInfo(CX, Loc, AAQIP);
+}
----------------
No clear?


================
Comment at: lib/Analysis/AliasAnalysis.cpp:598
                                     const MemoryLocation &Loc) {
+  return getModRefInfo(RMW, Loc, AAQIP);
+}
----------------
No clear?


================
Comment at: lib/Analysis/MemorySSA.cpp:1803
+  if (Walker)
+    Walker->verify(this);
   // Previously, the verification used to also verify that the clobberingAccess
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > (I'd be equally happy with `getWalker()->verify(this)`, personally, since I agree that this is a tricky edge case :) )
> `getWalker()` is not `const`, and I wanted to keep the `const` qualifier on the `verifyMemorySSA()`. Initializing the walker when building, so I removed the check.
Would it be cleaner for the verify to build its *own* walker? It isn't in the fast path and that would insulate it from one or the other decision?

(There probably is a good reason to not do this, mostly asking to learn more about MemorySSA and the walker design.)


================
Comment at: lib/Analysis/MemorySSA.cpp:1173
       SkipWalker(nullptr), NextID(0) {
-  buildMemorySSA();
+  BatchAAResults BatchAA(*AA);
+  buildMemorySSA(BatchAA);
----------------
I'd add some comments about *why* we want a batch AA result when building typically.


================
Comment at: lib/Analysis/MemorySSA.cpp:1175
+  buildMemorySSA(BatchAA);
+  this->AA = AA;
+  // Also create walker here. Verification relies on its existence.
----------------
Add a comment explaining that we intentionally leave AA null above to ensure the build phase uses the batch AA and not accidentally this one?

Basically, a comment for the reader who comes along and thinks "i'll fix this code to just use the member initializer above" about why the unusual pattern is unusual. I like my Chesterton's fence with signs on it. ;]


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59315