[PATCH] D73032: [DependenceAnalysis] Memory dependence analysis internal caching mechanism is broken in presence of TBAA (PR42733).

[Evgeniy via Phabricator via llvm-commits]

ebrevnov marked 2 inline comments as done.
ebrevnov added a comment.

In D73032#1876719 <https://reviews.llvm.org/D73032#1876719>, @john.brawn wrote:

> LGTM, though with a suggestion for adjusting a comment to be clearer.
>
> I do think though that MemoryDependenceAnalysis could do with an overhaul, as it's by now accumulated enough little fixes here and there that figuring out what's going on is way more difficult than it should be. In particular we could do with making the cache state explicit in NonLocalPointerInfo, instead of trying to infer it from from the other members. I may try doing that myself, but unfortunately I'm swamped with other things and won't have a chance for at least a week or so.


Agree. There should be ways to make things easier for understanding...



================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:1086-1088
+        // The cache is cleared (in the above line) but list of visited blocks
+        // is not. That means the cache will be missing information about
+        // visited blocks, thus incomplete.
----------------
john.brawn wrote:
> The comment here (and below) is a bit confusing, as my initial reading of it was "If the list of visited blocks hasn't been cleared, wouldn't clearing it fix things?". As I understand it, the problem is actually: we may have visited some block and stored information for it in NonLocalDeps and this has now been lost, and as we only ever visit a given block once for a given pointer value this guarantees that the final result will be incomplete.
> 
> I think a better comment would be something like
> 
> ```
> // The cache is cleared (in the above line) so we will have lost information
> // about blocks we have already visited. We therefore must assume that
> // the cache information is incomplete.
> ```
I like your wording. Fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73032