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

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:279
@@ +278,3 @@
+  ///
+  /// Note that X is interpreted as a SCEV expression, and this predicate does
+  /// not imply that X is equal to the backedge taken count. This means that
----------------
Optional suggestion: I'd change this line to "Note that this predicate does not imply ... taken count".  The "that X is interpreted as a SCEV expression" sounds confusing on the first reading (since we've already established that X is a SCEV expression returned by getPredicatedBackedgeTakenCount.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:281
@@ +280,3 @@
+  /// 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
----------------
sanjoy wrote:
> sbaranga wrote:
> > 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.
> > 
> > 
> SGTM, what you have is fine.  I added some minor wording suggestions -- all of which are optional to apply.
Optional suggestion: I'd remove the "Wrap" from "nusw Wrap", since we already have "w" for "wrap" in "nusw".

================
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
----------------
sbaranga wrote:
> 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.
> 
> 
SGTM, what you have is fine.  I added some minor wording suggestions -- all of which are optional to apply.


http://reviews.llvm.org/D17201