[PATCH] Fix PR21694 - Revert improper use of divide in SCEV HowFarToZero computation
Andrew Trick
atrick at apple.com
Thu Dec 11 12:53:03 PST 2014
> 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>
More information about the llvm-commits
mailing list