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

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: include/llvm/Analysis/MemoryDependenceAnalysis.h:305-307
+  /// Cache storing single non-local def. It is set in the case non local def
+  /// would be found in function returning only local dependencies, so then
+  /// by calling function returning nonLocal
----------------
I have hard time parsing this comment, I think you can remove the `so then` part or remove it altogether.


================
Comment at: include/llvm/Analysis/MemoryDependenceAnalysis.h:449
+  /// store the same value and NonLocal which indicate that non-local Def was
+  /// found, which can be retreived by calling getNonLocalPointerDependency
+  /// with the same quered instruction.
----------------
nitpicking, typos: 
retreived -> retrieved
quered -> queried.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:344
+
+  assert(InvariantGroupDependency.isUnknown());
+  return SimpleDep;
----------------
please add an assert message.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:371
   TryInsertToQueue(LoadOperand);
+
   while (!LoadOperandsQueue.empty()) {
----------------
spurious newline.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:911
   Result.clear();
-
+  {
+    auto NonLocalDef = NonLocalDefsCache.find(QueryInst);
----------------
why do you need a different scope?


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:912
+  {
+    auto NonLocalDef = NonLocalDefsCache.find(QueryInst);
+    if (NonLocalDef != NonLocalDefsCache.end()) {
----------------
The type of `NonLocalDef`  is not entirely obvious so I'd prefer if you use the explicit one.


================
Comment at: test/Transforms/NewGVN/invariant.group.ll:345-347
+; CHECK-LABEL: define void @handling_loops()
+define void @handling_loops() {
+  %a = alloca %struct.A, align 8
----------------
Thanks for adding NewGVN tests (although they're still XFAIL'ed, alas).


https://reviews.llvm.org/D28137