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

[Gerolf Hoflehner]

Ok, I changed the pass name to MergedLoadStoreMotion. From all suggestions it seemed it should contain at least ‘LoadStore'.

Also followed up on all other suggestions, too. Please see below.

-Gerolf

-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlsm.patch
Type: application/octet-stream
Size: 31805 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140706/61038af5/attachment.obj>
-------------- next part --------------




On Jun 24, 2014, at 7:20 AM, Chandler Carruth <chandlerc at gmail.com> wrote:

> 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…
Agree. Done.
> 
> ================
> Comment at: lib/Transforms/Scalar/InstructionMerger.cpp:1
> @@ +1,2 @@
> +//===---                  InstructionMerger.cpp                         ---===//
> +//
> ----------------
> This doesn't match the header format of LLVM files…
Done
A job for clang-format? 
> 
> ================
> 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 oxygen?
Done
> 
> ================
> 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.
Agree. Done.
> 
> ================
> 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).
Improved. Left review pointers. Added relevant TODOs.
> 
> ================
> 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.
Done.
> 
> (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.
Done.
> http://reviews.llvm.org/D4096
> 
>