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

[Chandler Carruth]

My biggest concern is the name. I don't think "InstructionMerge" is a great name because it *only* handles loads and stores, and the CFG motion triggered isn't really clarified. For example, what is different between merging instructions and combining them? Why isn't this in instcombine? I assume that the answer is that merging isn't the critical piece, but the hoisting and sinking is. I think naming it along those lines would clarify things. LoadStoreMotion or something seems appropriate and highlights the similarity between LICM pointed out by others in the review.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:210
@@ -209,2 +209,3 @@
 
+  MPM.add(createInstructionMergerPass()); // Merge load/stores in diamonds
   if (OptLevel > 1)
----------------
I wonder if this should be behind the O2 guard like GVN? It does seem to be in a similar class of transforms...

================
Comment at: lib/Transforms/Scalar/InstructionMerger.cpp:1
@@ +1,2 @@
+//===---                  InstructionMerger.cpp                         ---===//
+//
----------------
This doesn't match the header format of LLVM files...

================
Comment at: lib/Transforms/Scalar/InstructionMerger.cpp:10
@@ +9,3 @@
+//
+// This pass performs merges of loads and stores on both sides of
+// a diamond (hammock). It iteratively hoists two loads to the same
----------------
Maybe make this a \file comment for doxygen?

================
Comment at: lib/Transforms/Scalar/InstructionMerger.cpp:30-34
@@ +29,7 @@
+//            if.then:                         if.else:
+//               LOAD IT = [ADDR_L]              LOAD IE = [ADDR_L]
+//               <USE IT>                        <USE IE>
+//               <...>                           <...>
+//               STORE XT = [ADDR_S]             STORE XE = [ADDR_S]
+//               BR label %footer                BR label %footer
+//                     \                         /
----------------
I think it would be a lot nicer to actually use LLVM IR-style pseudo code here. The 'LOAD IT = ...' is particularly weird in LLVM.

================
Comment at: lib/Transforms/Scalar/InstructionMerger.cpp:62-79
@@ +61,20 @@
+//
+//===----------------------- Code Reviews/Feedback ------------------------===//
+//
+// Where/How to find them:
+//
+// a) LLVM commit mailing list:  [PATCH] Review for hoisting and sinking of
+// equivalent memory instruction (Instruction Merge Pass)
+// b) Phabricator: http://reviews.llvm.org/D4096
+//
+// Open Suggestions:
+// a) Integrate into GVN framework (see Dan Berlin's comments)
+//    -- Requires significant advances of current framework.
+// b) Generalize to regions other than diamonds and be more aggressive
+//    merging memory operations (comments by Philip Reames).
+//    -- I think this should be done on demand. Likely will require some
+//       way to measure register pressure at the IR level perhaps in form
+//       of a "pseudo-allocator".
+//
+//===----------------------------------------------------------------------===//
+
----------------
I don't think this really belongs in the code. It is way too transient to be useful. I would just have a TODO list for future work, or something similar. It needs to be phrased in a way that makes it clear that it is a persistently correct comment (until someone comes in and addresses / changes one of the specific issues).

================
Comment at: lib/Transforms/Scalar/InstructionMerger.cpp:170
@@ +169,3 @@
+
+/// Remove instruction from parent and update memory dependence analysis.
+void InstructionMerger::removeInstruction(Instruction *Inst) {
----------------
Brief doxygen should probably be tagged as \brief.

(This applies throughout the file.)

================
Comment at: lib/Transforms/Scalar/InstructionMerger.cpp:542
@@ +541,3 @@
+}
+/// runOnFunction - This is the main transformation entry point for a function.
+bool InstructionMerger::runOnFunction(Function &F) {
----------------
Skip the function name repetition.

http://reviews.llvm.org/D4096