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

Thu Oct 27 07:57:44 PDT 2022

nikic added a comment.

In D136175#3885870 <https://reviews.llvm.org/D136175#3885870>, @reames wrote:

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

I think for the purposes here, the only thing to know is that anything that influences BasicAA results should be part of the cache key.

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

Conceptually, yes. In practice, this is likely not strictly true in some edge cases. E.g. it could be that the queries start at different depths and one of them hits a depth cutoff and returns a worse result because of that. As such, I don't think we can actually assert it.

Based on your comments, I had the idea that instead of including this in the cache key, we could instead store both Result and ResultInCycle and if we for example determine that Result is MayAlias, we can directly set ResultInCycle to MayAlias as well, because we know it's not going to be better than that. Conversely, if ResultInCycle is NoAlias/MustAlias, we can also set Result based on that. I thought this would be pretty nice, in that it might avoid some redundant calculation. However, we have some pretty complex machinery for invalidating cache entries based on disproven noalias assumptions, and I think getting the interaction there right would be pretty tricky.

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

https://reviews.llvm.org/D136175