[PATCH] D98222: [SCEV] Use trip count information to improve shift recurrence ranges

Max Kazantsev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Mar 14 21:33:16 PDT 2021


mkazantsev added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5665
+    return CR;
+
+  unsigned TC = getSmallConstantMaxTripCount(L);
----------------
mkazantsev wrote:
> reames wrote:
> > reames wrote:
> > > mkazantsev wrote:
> > > > Are we missing a check that another operand is loop-invariant, or it is implied by something?
> > > We are, good catch.
> > Actually, now that I think about it further, we don't need the check. 
> > 
> > Consider the non-invariant case.  The known bits must hold for step on each iteration individually.  As a result, they must be a conservative result for the value of step on all iterations.  As such, if step is loop variant, we'll get a loose bound on the total step.
> > 
> > Instead of adding a bailout, I'll add a test for this case.
> The problem is not with this implementation, the problem is with the way how this is defined. What I understood from the comments around is that a shift recurrence is something analogous to the add recurrence. However, add recurrence assumes that all components of it are loop-invariant. If you run `opt -analyze -scalar-evolution` on
> 
> ```
> declare i1 @cond()
> 
> define void @foo(i32* %p) {
> entry:
>   %x = load volatile i32, i32* %p
>   br label %loop
> 
> loop:
>   %iv = phi i32 [0, %entry], [%iv.next, %loop]
>   %iv.next = add i32 %iv, %x
>   %cond = call i1 @cond()
>   br i1 %cond, label %loop, label %done
> 
> done:
>   ret void
> }
> ```
> 
> then you get addrec for %iv. But if you modify test to
> ```
> declare i1 @cond()
> 
> define void @foo(i32* %p) {
> entry:
>   br label %loop
> 
> loop:
>   %iv = phi i32 [0, %entry], [%iv.next, %loop]
>   %x = load volatile i32, i32* %p
>   %iv.next = add i32 %iv, %x
>   %cond = call i1 @cond()
>   br i1 %cond, label %loop, label %done
> 
> done:
>   ret void
> }
> ```
> then IV is a SCEVUnknown. The first comment inside this function suggests that what you are trying to recognize is analogous to the AddRec, but in fact it's not.
> 
> I don't say that the way how it works is wrong, I'm saying that if the pattern recognized is semantically different from AddRec, you can't just say that you are looking for something like "a simple recurrence of the form: <start, ShiftOp, Step>". This needs a better clarification of what `Step` actually is.
> 
> My preference is to avoid any kind of inconsistency in way how we define different recurrencies. If you support invariant step here, it should also be supported for AddRec beforehand.
> My preference is to avoid any kind of inconsistency in way how we define different recurrencies. If you support invariant step here, it should also be supported for AddRec beforehand.

sorry, I mean variant step off course.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98222/new/

https://reviews.llvm.org/D98222



More information about the llvm-commits mailing list