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

David Li via llvm-commits llvm-commits at lists.llvm.org
Sat Feb 27 15:43:49 PST 2016

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.


More information about the llvm-commits mailing list