[PATCH] D28137: [Devirtualization] MemDep returns non-local !invariant.group dependencies

[Daniel Berlin via Phabricator via llvm-commits]

dberlin accepted this revision.
dberlin added a comment.

The N^2 loop worries me a lot, because it means i can easily construct cases where you've made memdep N^3 or N^4 in the case of invariant group dependencies.

But i guess we'll see.



================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:333
+        return InvariantGroupDependency;
     }
   }
----------------
Please try to separate out the renaming from the actual logic changes


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:340
+  // Non-local invariant group dependency indicates there is non local Def,
+  // which is better than local clobber and everything else.
+  if (InvariantGroupDependency.isNonLocal())
----------------
Err, doesn't it simply indicate there *may* be a non-local def because it hit the top of the block?
(that's what it indicates for everything else)
How does it indicate there *must* be one?



================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:379
+      return Other;
+    return Best;
+  };
----------------
This loop is N^2, because each of these dominates calls make take O(N) time.



================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:429
+                            MemDepResult::getDef(ClosestDependency), nullptr));
+  return MemDepResult::getNonLocal();
 }
----------------
As long as you are willing to commit to doing this right, i'm okay with it.


https://reviews.llvm.org/D28137