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

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5162-5166
@@ -5109,1 +5161,7 @@
 
+      // If instructions A and B cannot be reordered because they're in a
+      // dependence relation, 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. We check that B is a write because our code motion won't
+      // ever violate WAR dependences.
+      if (B->mayWriteToMemory() && !canReorderMemAccesses(B, A)) {
----------------
anemet wrote:
> mssimpso wrote:
> > anemet wrote:
> > > Here I think we're validating the correctness of:
> > > 
> > >   1. Potentially moving 'A' an interleaved load before any store 'B' or
> > >   2. Potentially moving 'B' an interleaved store after any load/store 'A'.
> > > 
> > > If I am right, I don't think either the comment or the code is tight enough to reflect this.
> > > 
> > > It would also be good to add testcases for this.
> > > 
> > > It would be also great to add a testcase to the earlier problem that only candidate accesses were dependency-checked.
> > > 
> > > What do you think?
> > > 
> > Adam,
> > 
> > I think you're mostly right in the description of what we're validating the correctness of. When you have a chance, would you mind elaborating a bit more as to why you think we're not satisfying both of the cases you mention? The algorithm works bottom-up, so B will always precede A in program order, and we always investigate B's that are closest to A first (some dependences do temporarily slip by but are later checked, see below). Also note that we don't yet handle loops with predicated blocks. I'm working on that in D19694.
> > 
> > For case (1) the bottom-up ordering implies that if we can move a load A before a store B, we know that we can also move A before any store that B precedes. Case (2) is a bit more subtle and is probalby confusing. Say we have the case below:
> > 
> > ```
> > S1: Store  // depends on L1
> > L1: Load   // depends on S1
> > S2: Store
> > ```
> > 
> > When A is S2 and B is S1, we might say that these are independent, and S1 could be sunk to S2's location. We will add S1 to S2's group. But when A is L1 and B is S1, we will notice the dependence. When we do, we check to see if S1 is already in a group, and if so, invalidate it. This prevents us from moving S1.
> > 
> > Please let me know if I'm still missing something here. We could probably improve the comment and/or rename "canReorderMemAccesses", since this doesn't quite capture what's happening with the instruction ordering.
> > 
> > And yes, we can definitely add more tests.
> > I think you're mostly right in the description of what we're validating the correctness of. When you have a chance, would you mind elaborating a bit more as to why you think we're not satisfying both of the cases you mention?
> 
> I said "not tight enough" not that it's incorrect, sorry if that was unclear.
> 
> I think I would like to include the above two cases in the comment (or an improved version) and then have the checks in the code reflect that.   I.e. right now we don't check that at least one of 'A' or 'B' is interleaved.
> 
> > The algorithm works bottom-up, so B will always precede A in program order, and we always investigate B's that are closest to A first (some dependences do temporarily slip by but are later checked, see below). Also note that we don't yet handle loops with predicated blocks. I'm working on that in D19694.
> > 
> > For case (1) the bottom-up ordering implies that if we can move a load A before a store B, we know that we can also move A before any store that B precedes. Case (2) is a bit more subtle and is probalby confusing. Say we have the case below:
> > 
> > S1: Store  // depends on L1
> > L1: Load   // depends on S1
> > S2: Store
> > When A is S2 and B is S1, we might say that these are independent, and S1 could be sunk to S2's location. We will add S1 to S2's group. But when A is L1 and B is S1, we will notice the dependence. When we do, we check to see if S1 is already in a group, and if so, invalidate it. This prevents us from moving S1.
> 
> Make sense.
> 
> I am wondering now if there is a simpler way to formulate this analysis with the same result.  Aren't we simply saying that we don't consider an interleaved access for merging if it's either a source or the destination of a dependence?  I.e. something like this in the outer loop:
> 
> 
> ```
> if (isDepSource(A) && A->mayWriteToMemory() || isDepDestination(A))
>   break:
> ```
> 
> and then no need for the inner loop?
> 
> > Please let me know if I'm still missing something here. We could probably improve the comment and/or rename "canReorderMemAccesses", since this doesn't quite capture what's happening with the instruction ordering.
> > 
> > And yes, we can definitely add more tests.
> 
> Thanks!
> 
> 
> I think I would like to include the above two cases in the comment (or an improved version) and then have the checks in the code reflect that. I.e. right now we don't check that at least one of 'A' or 'B' is interleaved.

I see, yes this makes sense.

> I am wondering now if there is a simpler way to formulate this analysis with the same result. Aren't we simply saying that we don't consider an interleaved access for merging if it's either a source or the destination of a dependence?

This sounds right to me. Let me think it over before posting another update. Thanks again for all the feedback, Adam!




http://reviews.llvm.org/D19984