[PATCH] Fix PR21694 - Revert improper use of divide in SCEV HowFarToZero computation

[Andrew Trick]

> On Dec 11, 2014, at 12:40 PM, Mark Heffernan <meheff at google.com> wrote:
> 
> On Thu, Dec 11, 2014 at 1:26 AM, Andrew Trick <atrick at apple.com <mailto:atrick at apple.com>> wrote:
> Sorry I didn't respond. I wasn't able to easily convince myself that this fixes the problem. Since this reverts to the previous implementation, I think it's a good change. But the code is written in a way that is extremely hard to reason about.
> For example:
> - StepV could be negative but still be a power of two, is this expected?
> 
> What the ispowerof2 function returns isn't totally clear from its name.  It returns true if the number as an *unsigned* value is a power of 2, that is, it has exactly one bit set.

Except for the INT_MIN corner case, which you’re aware of from your comments.

> - Why are we checking GetMinTrailingZeros(getNegativeSCEV(Start)) instead of GetMinTrailingZeros(Distance)?
> 
> Isn't this expression always true:  GetMinTrailingZeros(foo) == GetMinTrailingZeros(getNegativeSCEV(foo))  ?  If so, it doesn't matter whether we use Start or

Indeed, which makes it very misleading to call getNegativeSCEV here. I would use “Distance” everywhere that we’re assuming we have a nonnegative value.

> getNegativeSCEV(Start) and distance necessarily is one of those values.  Given that, it probably makes sense to use a non-negated form, that is GetMinTrailingZeros(Start) or GetMinTrailingZeros(Distance) like you suggest.
> 
> Looking at the getUdivExpr:
> 
> const SCEV *Exact =
>   getUDivExpr(Distance, CountDown ? getNegativeSCEV(Step) : Step);
> 
> This is unnecessarily obfuscated.  It is equivalently:
> 
> const SCEV *Exact =
>   getUDivExpr(CountDown ? Start : getNegativeSCEV(Start), CountDown ? getNegativeSCEV(Step) : Step);
> 
> Seems like this can be expressed as:
> 
> const SCEV *Exact = getUDivExpr(Start, getNegativeSCEV(Step));
> 
> If we can only handle counting up here, lets be more clear about that.
> 
> There is only one case of counting down that we can handle here given the ispowerof2 constraint and that is when step is the maximally negative value (eg, INT_MIN).  And in this case Start must be either 0 or equal to step (maximally negative).

Exactly, so why complicate the code by handling this case. Why not guard the whole optimization with !CountDown for simplicity?

> If these cleanups sound reasonable to you I can make the change and add more comments.

That would be great! If some assumption is being made at a particular point, like a certain variable must be positive or negative, then there should be either an explicit test, assert, or at least a comment explaining why it’s safe to assume.

Thanks!

-Andy

> Mark
> 
> 
> 
> 
> 
> 
> http://reviews.llvm.org/D6546 <http://reviews.llvm.org/D6546>
> 
> 
> 
> EMAIL PREFERENCES
> 
>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
> 
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141211/030c724c/attachment.html>