[PATCH] D28044: [LV/LoopAccess] Check statically if an unknown dependence distance can be proven larger than the loop-count

[Dorit Nuzman via Phabricator via llvm-commits]

dorit added a comment.

In https://reviews.llvm.org/D28044#661369, @sanjoy wrote:

> I did not understand why this works (I've added some questions inline).  Some "constructive" reasoning (not necessarily a proof) for why `isSafeDependenceDistance` is correct may help (i.e. we know X and therefore we know Y etc.).  If this is already documented / discussed elsewhere then a reference to that discussion / documentation will be helpful too.


Thanks for looking closely into this.

I guess clarifying in the documentation that "Step" is the *absolute* stride would help… And I can also add the following:

"
We check here if the absolute distance (|Dist/Step|) is <=  the loop iteration count.
This is equivalent to the Strong SIV Test (Practical Dependence Testing , Section 4.2.1);

Note, that for vectorization it is sufficient to prove that the dependence distance is >= VF; This is checked elsewhere.
But in some cases we can prune unknown dependence distances early, and even before selecting the VF, and without a runtime test, by comparing the distance against the loop iteration count. Since the vectorized code will be executed only if LoopCount >= VF, proving that the distance >= LoopCount also guarantees that distance >= VF."



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1225
+  // If we can prove that
+  //      (**) |Dist| > BackedgeTakenCount * Step
+  // then there is no dependence.
----------------
sanjoy wrote:
> How does this work with a negative `BackedgeTakenCount * Step`?
> 
> For instance, in:
> 
> ```
> i = 0;
> do {
>   r0 = out[i + 1];
>   out[i] = r1;
> } while (--i != -2);
> ```
> 
> `D` is `1` (or `-1`), `BackedgeTakenCount` is `2` and `Step` is `-1`,
> so `|Dist| > BackedgeTakenCount * Step` is true (I assume you intend
> to use a signed `>` ?).
> 
> However, on the first iteration we write to `out[0]`, and we read from
> it on the second iteration.
> 
> Or is this somehow not the kind of dependence you're looking for?
Step cannot be negative. It is the absolute Stride in bytes. I will clarify that in the function description.



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1237
+  if (DistTypeSize > ProductTypeSize)
+    CastedProduct = SE.getZeroExtendExpr(Product, Dist.getType());
+  else
----------------
sanjoy wrote:
> Can you add a comment for why you're zero extending `Product` but sign extending `Dist`?
Is this clear enough?: 
"The dependence distance is signed (can be positive/negative), so we sign extend Dist; The (unsigned) product is the multiplication of the absolute stride in bytes, and the backdgeTakenCount, so we zero extend Product."



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1241
+
+  // Is  Dist - (BackedgeTakenCount * Step) > 0 ?
+  // (If so, then we have proven (**) because |Dist| >= Dist)
----------------
sanjoy wrote:
> How do you know that `Dist - (BackedgeTakenCount * Step)` won't
> overflow?
> 
> I.e. why can't `Dist` be `i8 128` and `BackedgeTakenCount * Step` be
> `i8 1`?
> 
> Skimming through LAA, it looks like it guards against overflow in the
> addressing expressions themselves, but I think the situation above can
> happen even if addressing expressions themselves do not overflow.
> 
> ```
> for (i8 i = 0; i < 2; i++) {
>   r0 = out[i - 64];
>   out[i + 64] = r1;
> }
> ```
> 
I'm not sure I understand the overflow concern...   First of all, just to make sure it's clear: we reach this code only when the distance is unknown,  so the specific example you gave with a known distance of 128 will not reach this code. So say the example was the following:

void foo (char *out, signed char Two, signed char SixtyFour) {
  for (signed char i = 0; i < Two; i++) {
    char r0 = out[i - SixtyFour];
    out[i + SixtyFour] = r0 * 2;
  }
}

The Dist that we will have is:   (2 * (sext i8 %SixtyFour to i64))
The Minus expressions that we will compute are:
Minus = (0 + (2 * (sext i8 %SixtyFour to i64)) + (-1 * (zext i8 %Two to i64))<nsw>)
Minus = (0 + (-2 * (sext i8 %SixtyFour to i64)) + (-1 * (zext i8 %Two to i64))<nsw>)

(And of course in this case we will not be able to prove statically that this is positive, so we will return that the distance is not safe).


https://reviews.llvm.org/D28044