[PATCH] D19984: [LV] Preserve order of dependences in interleaved accesses analysis

[Adam Nemet via llvm-commits]

anemet added a comment.

I am not done reviewing this but I just want to say that I am having a much better feeling about this analysis after your changes.  As I said I had major issues with its soundness.  Thanks for your patience for working through all this!

I am also sending my initial comments but there may be more coming.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:947-949
@@ -934,1 +946,5 @@
+
+  /// \brief Returns true if memory accesses \p A and \p B can be reordered. We
+  /// use LoopAccessInfo to determine if any dependences may be violated.
+  bool canReorderMemAccs(Instruction *A, Instruction *B);
 };
----------------
Somewhere this should state the criteria for reordering.  In theory you can reorder backward dependences but looks like you don't allow any (which is fine).

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5058
@@ -5036,3 +5057,3 @@
 
     StrideAccesses[I] = StrideDescriptor(Stride, Scev, Size, Align);
   }
----------------
I am wondering if it's time to change the name of this.  After your change it no longer contains only "interesting" strided accesses.  How about like AccessStrideInfo?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5062
@@ -5040,2 +5061,3 @@
 
-// Analyze interleaved accesses and collect them into interleave groups.
+bool InterleavedAccessInfo::canReorderMemAccs(Instruction *A, Instruction *B) {
+
----------------
I think that we've been using Accesses pretty consistently without any abbreviation.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5093-5095
@@ +5092,5 @@
+// location of the last store. This code motion can change the order of load
+// and store instructions and may break dependences. The inserted run-time
+// pointer aliasing checks ensure the absence of some dependences. However, we
+// need to explicitly check that we won't violate the dependences allowed for
+// vectorization.
----------------
I would drop the part about memchecks.  This is already pretty long and that part is trivial.


http://reviews.llvm.org/D19984