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

[Mark Heffernan]

On Thu, Dec 11, 2014 at 1:26 AM, Andrew Trick <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.

- 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 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).

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

Mark


>
>
>
>
> http://reviews.llvm.org/D6546
>
>
>
> EMAIL PREFERENCES
>
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141211/b8a2d667/attachment.html>