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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:752
@@ +751,3 @@
+/// This does one-way checks to see if Use could theoretically be hoisted above
+/// MaybeDef. This will not check the other way around.
+///
----------------
MaybeDef --> MayDef

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:759
@@ +758,3 @@
+                                            const Instruction *MayDef) {
+  // We only check for load-load clobbers; everything else is AA's problem. :)
+  if (!isa<LoadInst>(MayDef) || !isa<LoadInst>(Use))
----------------
The problem with the current interface design is that 'false' does not really mean 'false' -- it means either 'don't know yet' or 'no it can not be reordered'. This either leads to redundant check later (if it means 'no') or skipped mistakenly later (if it means 'don't know yet) when the client code does know the difference.  Possible solution is to move the check that both accesses are loads outside this function and change the function name to : canLoadsBeSafelyReordered( ...)

 if (isLoad(Def) && isLoad(Use)) 
   return canLoadsBeSafetlyReordered();

 // Using AA interface to do the check here 
  ...



================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:773
@@ +772,3 @@
+  // monotonic (read: std::memory_order_relaxed) loads, even if they alias.
+  return LoadDef->getOrdering() <= Monotonic &&
+         LoadUse->getOrdering() <= Monotonic;
----------------
This is too strict.

if (LoadDef->getOrdering() <= Monotonic && 
    LoadUse->getOrdering() <= Monotonic)
   return true;

 // Check other cases. For instance, non acquire loads before an acquire load can be moved after it.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:788-789
@@ -758,2 +787,4 @@
+      return false;
     return AA->getModRefInfo(DefMemoryInst, Loc) & MRI_Mod;
+  }
 
----------------
yes -- there should be an 'hand shaking' with AA interfaces otherwise you will need to special treatment here for synchronization related accesses before AA query. Add test cases to cover those should be good.


http://reviews.llvm.org/D16875