[PATCH] D12259: Improved Diagnostics and Extended vectorize(enable)

[Vedant Kumar via llvm-commits]

vsk added a comment.

Just nitpicks from me (comments inline).

I'd run git blame on some of these files and get one or two more reviewers.


================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:79
@@ -77,1 +78,3 @@
+  /// operations to vectorize.
+  static unsigned ForceVectorization;
   /// \brief VF as overridden by the user.
----------------
Nitpick, but from the coding style document: "Variable names should be nouns (as they represent state)". "VectorizationForced" might be a good compromise (and it's consistent).

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:30
@@ -29,1 +29,3 @@
 
+static cl::opt<unsigned, true> ForceVectorization(
+    "force-vectorization", cl::Hidden,
----------------
Same nitpick, see above.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:933
@@ +932,3 @@
+    return LV_NAME;
+  }
+  bool allowReordering() const {
----------------
It'd be helpful to restructure this code to return early when possible. E.g;

if (getWidth() == 1) return LV_NAME
if (getForce() == LVH::Disabled) return LV_NAME
if (getForce() != LVH::Enabled && getWidth() <= 1 && getInterleave() <= 1) return LV_NAME
return DI:AlwaysPrint

You may have to double-check that logic. Also, the third conditional looks like it could be simplified (given the first two).

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:934
@@ +933,3 @@
+  }
+  bool allowReordering() const {
+    return getForce() == LoopVectorizeHints::FK_Enabled || getWidth() > 1;
----------------
Please add a newline (and perhaps a comment?) before allowReordering().


http://reviews.llvm.org/D12259