[PATCH] D93529: [GVN][BasicAA] Enable clobbering in GVN.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/AliasAnalysis.cpp:950
+    }
+    return AAQI.getClobberOffset({V2, V1});
+  }
----------------
Why is it okay to treat the offset symmetrically? I'd expect the offset either be negative if swapped, or be not defined at all, depending on the contract of this function is supposed to be (probably needs a doc comment).


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1505
 
+  AAQI.VisitedPhi = true;
   // In the recursive alias queries below, we may compare values from two
----------------
Why do you use a separate VisitedPhi flag, rather than checking !VisitedPhiBBs.empty(), as we do elsewhere?

This kind of flag breaks BatchAA, because it will leak across queries.


================
Comment at: llvm/lib/Analysis/CaptureTracking.cpp:417
+    const Value *V, AAQueryInfo::IsCapturedCacheT *IsCapturedCache) {
+  AAQueryInfo::IsCapturedCacheT::iterator CacheIt;
   if (IsCapturedCache) {
----------------
This looks unrelated, land separately?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93529