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

[Arnold Schwaighofer]

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:72
@@ +71,3 @@
+// b) Phabricator: http://reviews.llvm.org/D4096
+//
+//===----------------------- TODO -----------------------------------------===//
----------------
I don't think we should reference this in the file.

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:266
@@ +265,3 @@
+      break;
+    if (isa<LoadInst>(Inst)) {
+      if (Inst->isUsedOutsideOfBlock(Inst->getParent()))
----------------
Less nested control flow (also makes the logic easier to read):

  if (!isa<LoadInst>(Inst))
    continue;
  if (Inst->isUsedOutsideOfBlock(Inst->getParent()))
    continue;

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:324
@@ +323,3 @@
+      (A0->getParent() == L0->getParent()) && A1->hasOneUse() &&
+      (A1->getParent() == L1->getParent()) && isa<GetElementPtrInst>(A0)) {
+    DEBUG(dbgs() << "Hoist Instruction into BB \n"; BB->dump();
----------------
Are we guaranteed that the GEP is recursively movable (are the GEP's inputs defined in BB or in AO->getParent())?

Say we had:

  %g0 = gep ...
  %g1 = gep %g0
  %l = load %g1

Would this still work?


================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:359
@@ +358,3 @@
+      LoadInst *L0 = dyn_cast<LoadInst>(I);
+      if (L0->isSimple()) {
+        ++NLoads;
----------------
You can rewrite this with less nested control flow as:

  auto LI = dyn_cast<LoadInst>(I);
  if (!LI || !LI->isSimple())
    continue;

================
Comment at: lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:525
@@ +524,3 @@
+    // Sink move non-simple (atomic, volatile) stores
+    if (isa<StoreInst>(I)) {
+      StoreInst *S0 = dyn_cast<StoreInst>(I);
----------------
You can rewrite this with less control flow see above.

http://reviews.llvm.org/D4096