[PATCH] D22377: [SCEV] trip count calculation for loops with unknown stride

[Pankaj Chawla via llvm-commits]

pankajchawla added a comment.

Hi Sanjoy,

Please see my replies inlined.

Thanks,
Pankaj

In https://reviews.llvm.org/D22377#485160, @sanjoy wrote:

> Hi Pankaj,
>
> There are three issues with the current patch:
>
> - Even after proving a non-negative stride, you cannot generally assume a non-zero stride.  A zero stride is possible if the loop has a side effect in it (i.e. a volatile read or write, or some IO). If the loop does not have a side effect then running forever is undefined behavior, so proving the loop does not have side effects + `ControlsExit` should be enough to prove that stride is non-zero.  A good place to compute it is in a common function that also computes `loopHasNoAbnormalExits` at the same time (to avoid traversing the loop body twice).


loopHasNoAbonormalExits() calls isGuaranteedToTransferExecutionToSuccessor() which checks for volatile loads/stores. This function also has a comment inside implying that this function checks for side effects. What other checks do we need? I don't know what it takes to prove that the loop has no side effects. Can you please give me some pointers?

> - It looks like you're saying if `Start` - `Stride` < `RHS` then the loop cannot be a single iteration loop.  I think that is incorrect, what if we have:

>   - `Start` = `-1`, `Stride` = `-2`, `RHS` = `5` for an unsigned compare? `Start` - `Stride` = `1` which is u< `RHS`, but if the backedge is controlled by `{-1,+,-2}` u< `5` then it won't be taken even once.

>   - `Start` = `INT_MAX`, `Stride` = `-1`, `RHS` = `5` for a signed compare? `Start` - `Stride` = `INT_MIN` which is s< `RHS`, but if the backedge is controlled by `{INT_MAX,+,-1}` s< `5` then it won't be taken even once.

> 

>     I think the right precondition is `Start` < `RHS` -- if that precondition is true then we know that `{Start,+,Stride}` is true on the first iteration, and if `Stride` is negative then we'll keep executing the loop till `{Start,+,Stride}` wraps.

> - I don't think the logic as written is correct for unsigned compares.  I can legitimately have `{0,+,-1}<nuw>` u< `-1` execute one backedge (i.e. with a "negative" step).


I think there is some misunderstanding here. The reason we are checking for `Start` - `Stride` < `RHS` rather than `Start` < `RHS` is because the AddRec is coming from the incremented value of the IV used in the backedge.

Consider this for loop-

  for (i=start; i<end; i+=stride)
    A[i] = i

In LLVM's canonical form, it will look like this-

  i = start
  if (start < end) {
    do {
      A[i] = i;
      i += stride;    // We pick up AddRec from updated i which at this point is start + stride.
    } while (i < end)
  }

For your first example when the IV is `{-1,+,-2}` it cannot have nuw flag. 
Please let me know if I have misunderstood you.

> Finally, we should also do this in `howManyGreaterThans` for symmetry,

>  but that's a separate change and does not need to be addressed in this

>  revision.


Yes, we should but I cannot write a positive test for this case. The reason is that we do not propagate the nsw flag to the IV for cases like this-

  for (i=n; i>0; i+=s)
    A[i] = i;

I discovered two additional issues with the variants of the original loop mentioned in the description. I brought this up in the llvm-dev mailing list. One is the lack of nsw propagation which I just mentioned. The second issue seems to be related to canonicalization. So if we change the loop control condition from `i<n` to `i<=n`, ScalarEvolution is not able to compute the trip count.

The canonical form of this loop looks something like this-

  if (0 <= n) {
    i = 0;
    do {
      A[i] = i;
      i += s; // NSW add
    } while (! (i > n));  // sgt compare
  }

The 'sgt' compare is inverted to 'sle' for analysis. ScalarEvolution isn't really expecting 'sle' in canonicalized loops so it reverts to brute force exit count computation using computeExitCountExhaustively() which doesn't work.


https://reviews.llvm.org/D22377