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

[Michael Zolotukhin via llvm-commits]

mzolotukhin accepted this revision.
mzolotukhin added a reviewer: mzolotukhin.
mzolotukhin added a comment.
This revision is now accepted and ready to land.

Hi Tyler,

Thanks for the explanation - please find my replies inline. They are mostly cosmetic, so in general the patch looks good to me.

Michael


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:937-943
@@ -921,1 +936,9 @@
+
+  bool allowReordering() const {
+    // Allow the vectorizer to change the order of operations that is given by
+    // the scalar loop when it is potentially unsafe or inefficient. For
+    // example, changing the order of floating-point operations can affect the
+    // result due to differences in floating-point round-off. Specifying a loop
+    // hint instructs the vectorizer to proceed.
+    return getForce() == LoopVectorizeHints::FK_Enabled || getWidth() > 1;
   }
----------------
It's a bit unclear, what `getWidth()` has to do with reordering of operations. Do we basically check that the loop has any vectorization hint attached to it (and that it's not "don't vectorize this loop")? If my understanding is correct, could you please mention it in the comment somehow?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1537
@@ +1536,3 @@
+                                VectorizerParams::RuntimeMemoryCheckThreshold &&
+                            !Hints.allowReordering();
+    if (ThresholdReached || PragmaThresholdReached) {
----------------
I think I wasn't clear enough - I meant that while we do want to check this condition, it shouldn't  be computed as a part of `ThresholdReached` variable, because semantically it doesn't belong there. I meant that we just need to pull out  `&& !Hints.allowReordering` to the check below.


http://reviews.llvm.org/D12259