[PATCH] D16875: MemorySSA Optimizations: Patch 1 of N

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:711
@@ +710,3 @@
+/// MaybeDef, with no potentially clobbering operations in between them.
+/// Where potentially clobbering ops are memory barriers, aliased stores, ...
+static bool canUseBeReorderedAboveDef(const Instruction *Use,
----------------
Unfinished sentence here.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:713
@@ +712,3 @@
+static bool canUseBeReorderedAboveDef(const Instruction *Use,
+                                      const Instruction *MaybeDef) {
+  // We only check for load-load clobbers; everything else is AA's problem. :)
----------------
nit: MaybeDef -> MayDef

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:714
@@ +713,3 @@
+                                      const Instruction *MaybeDef) {
+  // We only check for load-load clobbers; everything else is AA's problem. :)
+  if (!isa<LoadInst>(MaybeDef) || !isa<LoadInst>(Use))
----------------
This needs some explanation --> can the alias query be moved here to make this function more general?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:724
@@ +723,3 @@
+
+  // ...But volatile operations can be reordered with non-volatile operations.
+  //
----------------
Move this comment up and combine with the other volatile related comment.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:744
@@ -713,1 +743,3 @@
+      return false;
     return AA->getModRefInfo(DefMemoryInst, Loc) & MRI_Mod;
+  }
----------------
This does not look correct:

When canUseBeReorderedAboveDef returns false, it is not ok to unconditionally feed it into the AA query -- unless AA query also honors the ordering constraint (I have not checked).

================
Comment at: test/Transforms/Util/MemorySSA/atomic-clobber.ll:16
@@ -14,3 +15,3 @@
   %2 = load i32, i32* %a, align 4
-  %3 = add i32 %1, %2
-  ret i32 %3
+  ret void
+}
----------------
what is this change about?

================
Comment at: test/Transforms/Util/MemorySSA/atomic-clobber.ll:41
@@ +40,3 @@
+  %1 = load atomic i32, i32* %a acquire, align 4
+; CHECK: MemoryUse(1)
+; CHECK-NEXT: %2 = load atomic i32
----------------
Is this correct? The atomic load can not be reordered before the previous load with acquire.

================
Comment at: test/Transforms/Util/MemorySSA/atomic-clobber.ll:44
@@ +43,3 @@
+  %2 = load atomic i32, i32* %a unordered, align 4
+; CHECK: 2 = MemoryDef(1)
+; CHECK-NEXT: %3 = load atomic i32
----------------
Is this too conservative?


http://reviews.llvm.org/D16875