[PATCH] D17201: [SCEV] Introduce a guarded backedge taken count and use it in LAA and LV

[silviu.baranga@arm.com via llvm-commits]

sbaranga added a comment.

In http://reviews.llvm.org/D17201#392057, @sanjoy wrote:

> Hi Silviu,
>
> This lgtm now!  Thanks for bearing with the long review process.  I have a minor "messaging" comment inline, but otherwise things look great!


Thanks a lot for reviewing! Some comments inline, but it looks like this shouldn't be too difficult to resolve.

-Silviu


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:281-282
@@ +280,4 @@
+  /// not imply that X is equal to the backedge taken count. This means that
+  /// if we have a nusw Wrap predicate for i32 {0,+,1} with a predicated
+  /// backedge taken count of X, we only guarantee that the {0,+,1} has nusw
+  /// in the first X iterations. {0,+,1} may still wrap in the loop if
----------------
sanjoy wrote:
> What I have in mind is slightly different (same content, said
> differently):
> 
>  1. If a SCEVPredicate says `AR` is `nssw` then `AR` *is* `nssw` for
>     the entire iteration space of the loop.  No exceptions.
> 
>  2. The predicated BE taken computation logic now needs to be aware of
>     a possible circular logic issue -- if it adds a predicate forcing
>     an `AR` to be `nssw` then `AR` is `nssw` only for the trip count
>     it itself computes.  This means it cannot use the no-overflow
>     property in certain ways the non-predicated BE taken computation
>     can.
> 
> If (1) is ever incorrect (i.e. the predicate fails to hold) then we've
> already "lost" (i.e. miscompiled) because
> `getPredicatedBackedgeTakenCount` gave us the wrong answer.
> 
> In other words, I think you're downplaying the fact that the "burden"
> is really on `getPredicatedBackedgeTakenCount`, and not on the general
> notion of predicate itself. For instance, there is no complication at
> all if we don't also use predicated backedge taken counts (i.e. we use
> normal backedge taken counts) with `SCEVPredicate`.
Yes, this was what I also had in mind initially.

However, I found it more easy to reason about things this way. Here is the reason:

The new definition also happens to imply the old one for all code that also checks the predicated backedge taken count, and it only makes a difference when computing this predicated backedge taken count.

It removes the circular logic issue for our current use case. That is, we don't need to know that the predicated backedge taken count is correct in order to check the predicate, so the reasoning would look like:

wrap predicate (+ possibly some other predicates) => predicated backedge taken count is correct
predicated backedge taken count is correct + wrap predicate => Wrap predicate is true throughout the loop

which looked like a slightly more elegant way of reasoning about this.

The burden is still on the getPredicatedBackedgeTakenCount to correctly use the predicates and not get itself into circular logic.

Also, we can still use the backedge taken count here since if we can compute it, the predicated backedge taken count would be equal to it and have no predicates.

Given this, what do you think? If you still like to go with your version, we can do that.




http://reviews.llvm.org/D17201