[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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Stylistic comments only for the moment.  I'm trying to wrap my head around the revised invariants.  On first thought, I'd still prefer my earlier suggestion, but I haven't full understood the objection.  I'll try to spend some more thinking time on this tomorrow.



================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:981
 
+  NonLocalDepEntry *ExistingResult = nullptr;
+  bool isInvariantLoad = false;
----------------
Please move this definition back to where it was as nothing in the new scope uses it.


================
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
----------------
I'd suggest structuring the bit the follows as a conditional clear of ExistingResult here.

if (isInvariantLoad && !ExistingResult->getResult().isNonFuncLocal()))
  ExistingResult = nullptr;


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:1037
+      ExistingResult->setResult(Dep);
+    else
+      Cache->push_back(NonLocalDepEntry(BB, Dep));
----------------
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)


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64405