[PATCH] D36113: [Loop Vectorize] Vectorize Loops with Backward Dependence

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/Transforms/IPO/PassManagerBuilder.h:151
   bool LoopVectorize;
+  bool LoopVectorizePred;
   bool RerollLoops;
----------------
I don't think we need a special pass-manager option for this. We can put it behind a cl::opt flag, but once it is judged to be ready for the default pipeline, I imagine we'll always enable it whenever vectorization is enabled (at least at -O3).


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorizePred.h:10
+//
+// This is the LLVM pass which needs to be run before loop vectorizer. This pass
+// checks if there is a backward dependence between instructions ,if so, then it
----------------
This first sentence seems inaccurate. It does not need to be run, nor does it need to run before the vectorizer. You can say that it is intended to be run before the vectorizer.


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorizePred.h:11
+// This is the LLVM pass which needs to be run before loop vectorizer. This pass
+// checks if there is a backward dependence between instructions ,if so, then it
+// tries to reorders instructions, so as to convert non-vectorizable loop into
----------------
" instructions ,if so, " -> " instructions, and if so, "


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorizePred.h:13
+// tries to reorders instructions, so as to convert non-vectorizable loop into
+// vectorizable form and generates target-independent LLVM-IR. The vectorizer
+// uses the LoopAccessAnalysis  and MemorySSA  analysis to check for
----------------
"and generates target-independent LLVM-IR" - all passes do this unless other noted. No need to say that here.


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorizePred.h:13
+// tries to reorders instructions, so as to convert non-vectorizable loop into
+// vectorizable form and generates target-independent LLVM-IR. The vectorizer
+// uses the LoopAccessAnalysis  and MemorySSA  analysis to check for
----------------
hfinkel wrote:
> "and generates target-independent LLVM-IR" - all passes do this unless other noted. No need to say that here.
"The vectorizer uses" - Is this a statement about the vectorizer or this pass?


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorizePred.h:14
+// vectorizable form and generates target-independent LLVM-IR. The vectorizer
+// uses the LoopAccessAnalysis  and MemorySSA  analysis to check for
+// dependences, inorder to reorder the instructions.
----------------
 the LoopAccessAnalysis  -> remove "the" and the  extra spaces on this line.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:12
+// This is the LLVM pass which needs to be run before loop vectorizer. This pass
+// checks if there is a backward dependence between instructions ,if so, then it
+// tries to reorder instructions, so as to convert non-vectorizable loop into
----------------
See comments from the header file. They apply here too.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:17
+// dependences, inorder to reorder the instructions.
+
+//===----------------------------------------------------------------
----------------
Please put here some pseudo-cost/C examples, before/after, showing what the transformation does.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:57
+/// non-vectorizable loop into vectorizable form. It first finds out the store
+/// instructions which aliases with a load instruction,and then uses MemorySSA
+/// Analysis to check ,if it is legal to move a load instruction above the store
----------------
space before and


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:58
+/// instructions which aliases with a load instruction,and then uses MemorySSA
+/// Analysis to check ,if it is legal to move a load instruction above the store
+/// instructions. If it is legal, then it checks for all the dependencies of the
----------------
remove comma before if.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:111
+
+bool LoopInstructionsReordering::checkDepAndReorder(
+    BasicBlock::iterator StoreInstIter, BasicBlock::iterator LoadInstIter) {
----------------
Please add a comment explaining what this function does.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:113
+    BasicBlock::iterator StoreInstIter, BasicBlock::iterator LoadInstIter) {
+  bool reordered = false;
+  bool moved = true;
----------------
Local variable names need to start with a capital letter.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:116
+  BasicBlock::iterator currInstIter = StoreInstIter;
+  while (moved && (&*currInstIter != &*LoadInstIter)) {
+    // Reverse Iterating through all the instructions between backward
----------------
Is this algorithm quadratic?


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:118
+    // Reverse Iterating through all the instructions between backward
+    // dependent load  and store instructions , to check which other
+    // instructions which are operands of load instructions also needs to be
----------------
Please be careful with the formatting of comments. Only once space in between words. A single space goes after a comma, none before.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:130
+      unsigned opNum;
+      for (opNum = 0; opNum < (&*currInstIter)->getNumOperands(); opNum++) {
+        if (((&*currInstIter)->getOperand(opNum) == (&*rit)))
----------------
Can you do this with std::find and op_begin/op_end?


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:175
+      // If the Definition of the Load Instruction is live on Entry ,
+      // then that means it is not dependent on the store instructions
+      // and can be moved above the store instruction
----------------
It also makes the load loop invariant. Why does LICM not hoist it?


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:210
+
+static bool hasCyclesInLoopBody(const Loop &L) {
+  if (!L.empty())
----------------
Please describe what this function does.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:226
+
+static void addAcyclicInnerLoop(Loop &L, SmallVectorImpl<Loop *> &V) {
+  if (L.empty()) {
----------------
Please describe what this function does.


https://reviews.llvm.org/D36113