[PATCH] Review for hoisting and sinking of equivalent memory instruction (Instruction Merge Pass)

[Arnold Schwaighofer]

================
Comment at: lib/Transforms/Scalar/IM.cpp:162
@@ +161,3 @@
+  }
+  // FIXME: do we have to check for memory models?
+  // For example can we hoist above volatile loads?
----------------
Philip Reames wrote:
> Answer: yes.  You need to check for fences and load/stores which are ordered w.r.t. the one your moving.  (I think the second is actually handled by the isSimple check, but you definitely need the first.)
> 
> You're also missing invokes.  
The code checks for mayHaveSideEffects which covers fences (=> mayWriteToMemory returns true) and loads that have ordering (=> mayWriteToMemory returns true). Invokes are terminators.

Gerolf's comment is misleading though ...

================
Comment at: lib/Transforms/Scalar/IM.cpp:184
@@ +183,3 @@
+      break;
+    if (isa<LoadInst>(Inst)) {
+      if (Inst->isUsedOutsideOfBlock(Inst->getParent()))
----------------
Philip Reames wrote:
> You can't safely move ordered loads with the current code.
> 
> 
mayHaveSideEffects (isLoadHoistBarrier) returns true for loads with atomic ordering.

================
Comment at: lib/Transforms/Scalar/IM.cpp:213
@@ +212,3 @@
+  // Intersect optional metadata.
+  HoistCand->intersectOptionalDataWith(ElseInst);
+
----------------
Philip Reames wrote:
> Shouldn't this be done with the clone?
He is intersecting metadata from left and right.

Gerolf you will also need to  Instruction::dropUnknownMetadata.

http://reviews.llvm.org/D4096