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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I just had a quick look and left a few comments. I'm just curious, what kind of testing did you do & did you run any benchmarks?



================
Comment at: include/llvm/Transforms/Vectorize.h:128
+Pass *createLoopVectorizePredPass(bool NoUnrolling = false,
+                              bool AlwaysVectorize = true);
+
----------------
Indent is off.


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorizePred.h:2
+//===---- LoopVectorizePred.h ---------------------------------------*- C++
+//-*-===//
+//
----------------
This should go on line 1 I think, see first line of Vectorize.h for example.


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:831
     PM.add(createSimpleLoopUnrollPass(OptLevel));   // Unroll small loops
+  PM.add(createLoopVectorizePredPass(true, LoopVectorize));
   PM.add(createLoopVectorizePass(true, LoopVectorize));
----------------
I think people might be more comfortable with adding the new pass if there is an option to completely disable it, as in not adding it to the pass manager (like with LoopInterchange for example). This way, the pass can go in being disabled by default and makes it easier for people to test it/run benchmarks.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:111
+
+bool LoopInstructionsReordering::checkDepAndReorder(
+    BasicBlock::iterator StoreInstIter, BasicBlock::iterator LoadInstIter) {
----------------
I think this function could do with slightly more comments, e.g. what the loops are doing and so on.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:115
+  int moved = 1;
+  BasicBlock::iterator temprit = StoreInstIter;
+  while ((moved == 1) && (&*temprit != &*LoadInstIter)) {
----------------
Is there a more meaningful name instead of temprit?


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:152
+  for (auto Dep : *Deps) {
+    if ((Dep.Type == MemoryDepChecker::Dependence::DepType::Backward)) {
+
----------------
I think you could use Dep.isBackward(), which is shorter.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizePred.cpp:160
+        BasicBlock::iterator it = BB->begin();
+        while (it != BB->end()) {
+
----------------
Could be a for loop?


https://reviews.llvm.org/D36113