<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Sep 19, 2015 at 12:45 AM, Sanjoy Das <span dir="ltr"><<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Not objecting to this fix, I don't quite understand the<br>
AP2.isPowerOf2() caveat.<span class=""><br>
<br>
David Majnemer via llvm-commits wrote:<br>
>         if (Shift > 0) {<br>
>     -    if (IsAShr ? AP1 == AP2.ashr(Shift) : AP1 == AP2.lshr(Shift))<br>
>     +    if (IsAShr && AP1 == AP2.ashr(Shift)) {<br>
>     +      // There are multiple solutions if we are comparing against<br>
>     -1 and the LHS<br>
>     +      // of the ashr is not a power of two..<br>
>     +      if (AP1.isAllOnesValue() && !AP2.isPowerOf2())<br>
<br></span>
Firstly, for AP1 == -1 && IsAShr, AP2.isPowerOf2() will be true iff<br>
AP2 is INT_MIN, right (because of the (AP2.isNegative() !=<br>
AP1.isNegative())) above?<br>
<br>
And if I have (in i8): "(128 >> x) == -1" then all x >= 7 are<br>
solutions: x == 7 is clearly a solution and for everything greater<br>
than 7, the comparison is "undef == -1" == "true".<br>
<br>
Of course, you could also evaluate "undef == -1" to "false" (and so<br>
emitting an EQ is correct), but we don't really need the "&&<br>
!AP2.isPowerOf2()" for correctness, IIUC.</blockquote><div><br></div><div>The `isPowerOf2` bit is not for correctness, you are correct when you say that writing the comparison as >= in those cases would be correct.</div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888"><br>
<br>
-- Sanjoy<br>
</font></span></blockquote></div><br></div></div>