[PATCH] D136175: [BasicAA] Include MayBeCrossIteration in cache key

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Can you add some explanatory comments into the code please?  I'm not entirely sure I understand the caching protocol, and I doubt future readers will.  I think this is correct, but I'm not 100% sure on that.

It would also help if you'd explicitly state the invariant relating results with and without the cycle flag.  I *think* non-cyclic results should always be at least as precise as the potentially cyclic results.  Right?  If so, that feels like a valuable thing to note, and (maybe) assert?



================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:192
 
 /// Reduced version of MemoryLocation that only stores a pointer and size.
 /// Used for caching AATags independent BasicAA results.
----------------
Update comment please.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1442
-  // with a new AAQueryInfo.
-  AAQueryInfo NewAAQI = AAQI.withEmptyCache();
-  AAQueryInfo *UseAAQI = !SavedMayBeCrossIteration.get() ? &NewAAQI : &AAQI;
----------------
This appears to have been only use of withEmptyCache, can you remove?


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1575
   // otherwise infinitely recursive queries.
-  AAQueryInfo::LocPair Locs({V1, V1Size}, {V2, V2Size});
+  AAQueryInfo::LocPair Locs({V1, V1Size, MayBeCrossIteration},
+                            {V2, V2Size, MayBeCrossIteration});
----------------
Can you comment this please?


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

https://reviews.llvm.org/D136175