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

[Adam Nemet via llvm-commits]

anemet added a comment.

Matt,

All agreed, I just have a few more suggestions to improve comments for this complex piece.

Let me know what you think.

Adam


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5200-5207
@@ -5199,10 +5308,1 @@
   //   3. B belongs to the group according to the distance.
-  //
-  // The bottom-up order can avoid breaking the Write-After-Write dependences
-  // between two pointers of the same base.
-  // E.g.  A[i]   = a;   (1)
-  //       A[i]   = b;   (2)
-  //       A[i+1] = c    (3)
-  // We form the group (2)+(3) in front, so (1) has to form groups with accesses
-  // above (1), which guarantees that (1) is always above (2).
-  for (auto I = StrideAccesses.rbegin(), E = StrideAccesses.rend(); I != E;
----------------
Wow, this is very subtle too.  This and the previous case of why we need to remove elements from a group needs a high-level comment somewhere around the call to canReorder...  See my comments on the particular lines.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5334-5343
@@ +5333,12 @@
+
+      // If B and A cannot be reordered, we can't add additional instructions
+      // to A's group since this may cause illegal code motion. If B is already
+      // in a group release it.
+      if (!canReorderMemAccessesForInterleavedGroups(&*BI, &*AI)) {
+        if (isInterleaved(B)) {
+          InterleaveGroup *StoreGroup = getInterleaveGroup(B);
+          StoreGroups.remove(StoreGroup);
+          releaseGroup(StoreGroup);
+        }
+        break;
+      }
----------------
I don't think the comment is sufficient to cover all the subtleties here.  I would prefer to say something like:

We can't have dependences between accesses in a group and other accesses that are located between the first and the last element of group.

Probably before the break statement we should also say that:

It's OK to have dependences between accesses in a group and other accesses before the first instruction we just can't extend the group beyond these.

I am also wondering if we need a picture to further visualize this, i.e. a case where there is an intervening access and one where falls outside the range of the group.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5351-5354
@@ -5227,6 +5350,6 @@
 
       // Ignore if B is already in a group or B is a different memory operation.
       if (isInterleaved(B) || A->mayReadFromMemory() != B->mayReadFromMemory())
         continue;
 
       // Check the rule 1 and 2.
----------------
OK.

================
Comment at: test/Transforms/LoopVectorize/interleaved-accesses.ll:769-770
@@ +768,4 @@
+
+; PR27626_4: Ensure we form an interleaved group for strided stores in the
+;            presence of a write-after-write dependence.
+
----------------
... but exclude a[i] = x


http://reviews.llvm.org/D19984