[llvm] r248073 - [InstCombine] FoldICmpCstShrCst didn't handle icmps of -1 in the ashr case correctly

[David Majnemer via llvm-commits]

On Sat, Sep 19, 2015 at 12:45 AM, Sanjoy Das <sanjoy at playingwithpointers.com
> wrote:

> Not objecting to this fix, I don't quite understand the
> AP2.isPowerOf2() caveat.
>
> David Majnemer via llvm-commits wrote:
> >         if (Shift > 0) {
> >     -    if (IsAShr ? AP1 == AP2.ashr(Shift) : AP1 == AP2.lshr(Shift))
> >     +    if (IsAShr && AP1 == AP2.ashr(Shift)) {
> >     +      // There are multiple solutions if we are comparing against
> >     -1 and the LHS
> >     +      // of the ashr is not a power of two..
> >     +      if (AP1.isAllOnesValue() && !AP2.isPowerOf2())
>
> Firstly, for AP1 == -1 && IsAShr, AP2.isPowerOf2() will be true iff
> AP2 is INT_MIN, right (because of the (AP2.isNegative() !=
> AP1.isNegative())) above?
>
> And if I have (in i8): "(128 >> x) == -1" then all x >= 7 are
> solutions: x == 7 is clearly a solution and for everything greater
> than 7, the comparison is "undef == -1" == "true".
>
> Of course, you could also evaluate "undef == -1" to "false" (and so
> emitting an EQ is correct), but we don't really need the "&&
> !AP2.isPowerOf2()" for correctness, IIUC.


The `isPowerOf2` bit is not for correctness, you are correct when you say
that writing the comparison as >= in those cases would be correct.
However, `x >= 7` is not as tight a bound as `x == 7` and so I reckoned
that it'd only be necessary to emit the >= check when `!isPowerOf2` and
that == would be better in the abstract.


>
>
> -- Sanjoy
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150919/ddf496bf/attachment.html>