[PATCH] D64405: [DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses. Fix for https://bugs.llvm.org/show_bug.cgi?id=42151

[Evgeniy via Phabricator via llvm-commits]

ebrevnov marked 4 inline comments as done.
ebrevnov added inline comments.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:996
     ExistingResult = &*Entry;
 
   // If we have a cached entry, and it is non-dirty, use it as the value for
----------------
reames wrote:
> I'd suggest structuring the bit the follows as a conditional clear of ExistingResult here.
> 
> if (isInvariantLoad && !ExistingResult->getResult().isNonFuncLocal()))
>   ExistingResult = nullptr;
Even though I don't feel that requested structure is any better I have no objections against that change. Please note that ExistingResult should be checked for not being null as well.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:1037
+      ExistingResult->setResult(Dep);
+    else
+      Cache->push_back(NonLocalDepEntry(BB, Dep));
----------------
reames wrote:
> Per the above, ExistingResult must be null for an invariant load, so this can be rewritten as:
> 
> if (ExistingResult)
>       ExistingResult->setResult(Dep);
>  else if (!isInvariantLoad)
Taking into account your next comment I think we better just bail out for invariant loads.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:1470
 
-    bool foundBlock = false;
     for (NonLocalDepEntry &I : llvm::reverse(*Cache)) {
       if (I.getBB() != BB)
----------------
reames wrote:
> This bit looks to be NFC minus the removal of the assert?  Is that correct?
Absolutely.


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

https://reviews.llvm.org/D64405