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

[George Burgess IV via llvm-commits]

george.burgess.iv 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.
+///
----------------
davidxl wrote:
> MaybeDef --> MayDef
Oops. :)

================
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))
----------------
davidxl wrote:
> 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 
>   ...
> 
> 
Yeah, I like `canLoadsBeReordered` better.

================
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;
----------------
davidxl wrote:
> 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.
Nice catch.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:788-789
@@ -758,2 +787,4 @@
+      return false;
     return AA->getModRefInfo(DefMemoryInst, Loc) & MRI_Mod;
+  }
 
----------------
davidxl wrote:
> 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.
Added `check_aa_is_sane` (which is really tiny) in atomic-clobber.ll. I think it largely covers the behavior that we need, though.


http://reviews.llvm.org/D16875