[PATCH] D12992: invariant.group handling in GVN

[Nick Lewycky via llvm-commits]

nlewycky added inline comments.

================
Comment at: include/llvm/Analysis/MemoryDependenceAnalysis.h:400
@@ -399,3 +399,3 @@
 
-    /// getPointerDependencyFrom - Return the instruction on which a memory
+    /// \brief - Return the instruction on which a memory
     /// location depends.  If isLoad is true, this routine ignores may-aliases
----------------
I don't think that's "brief". "[A] brief description is a short one-liner" -- https://www.stack.nl/~dimitri/doxygen/manual/docblocks.html

================
Comment at: include/llvm/Analysis/MemoryDependenceAnalysis.h:420
@@ +419,3 @@
+
+    /// \brief - This analysis looks for other
+    /// loads and stores with invariant.group metadata and the same
----------------
Brief again.

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:399
@@ +398,3 @@
+  // It's is not safe to walk the uses of global value, because nobody guarantee
+  // that uses list won't change during iteration (uses list doesn't have
+  // any lock and there might be loads in different functions being optimized
----------------
"use list" not "uses list" (two occurrences in this line).

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:402
@@ +401,3 @@
+  // in the same time).
+  if (isa<GlobalValue>(Ptr))
+    return MemDepResult::getUnknown();
----------------
It's the 'function passes aren't allowed to look outside their functions' rule, as-if we had function passes running in parallel.

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:419
@@ +418,3 @@
+        U->getMetadata(LLVMContext::MD_invariant_group) == InvariantGroupMD)
+      return MemDepResult::getDef(U);
+  }
----------------
Hold up. If there's three loads, A B and C, all of them in the same invariant group, and C is the QueryInst for getPointerDependencyFrom, and A and B both dominate C, doesn't the value returned right here depend on the order of the use list? That seems undesirable, but I'm not sure whether that's considered a serious problem or not.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:2484
@@ -2478,2 +2483,3 @@
        BI != BE;) {
+
     if (!ReplaceWithConstMap.empty())
----------------
Spurious change

================
Comment at: lib/Transforms/Utils/Local.cpp:1432-1433
@@ +1431,4 @@
+  if (auto *JMD = J->getMetadata(LLVMContext::MD_invariant_group))
+    if (isa<LoadInst>(K) || isa<StoreInst>(K))
+      K->setMetadata(LLVMContext::MD_invariant_group, JMD);
+
----------------
Just move this part into the "case LLVMContext::MD_invariant_group:" ?

================
Comment at: test/Transforms/GVN/invariant.group.ll:146
@@ +145,3 @@
+
+; CHECK-LABEL:define void @loadCombine() {
+define void @loadCombine() {
----------------
Space after colon


http://reviews.llvm.org/D12992