<div dir="ltr"><div>Hi Nick,<br><br></div><div>Thanks a lot for the review :)<br></div><div><br></div>I think you missed review conversation with Duncan (It was my mistake that i forgot <br>to cc llvm-commit, but later did it. My sincere apology).<br>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Jul 6, 2014 at 7:34 AM, Nick Lewycky <span dir="ltr"><<a href="mailto:nicholas@mxc.ca" target="_blank">nicholas@mxc.ca</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
================<br>
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2349<br>
@@ +2348,3 @@<br>
<div class="">+      (cast<BinaryOperator>(Op)->isExact())) {<br>
</div><div class="">+    if (CI2->getValue().srem(CI->getValue()) == 0) {<br>
</div><div class="">+      Quotient = CI2->getValue().sdiv(CI->getValue());<br>
</div>----------------<br>
APInt::isMinValue() is faster than APInt::operator== even though it's less clear to read.<br>
<br>
================<br>
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2350<br>
@@ +2349,3 @@<br>
<div class="">+    if (CI2->getValue().srem(CI->getValue()) == 0) {<br>
</div><div class="">+      Quotient = CI2->getValue().sdiv(CI->getValue());<br>
+      return true;<br>
</div>----------------<br>
APInt::sdivrem is as fast as a single sdiv or srem. Please use it then text the 'rem' results.<br>
<br></blockquote><div><br></div><div>Duncan has suggested to calculate 'rem' and 'Shift' as following:<br><br></div><div>       <i>if(C1.isNegative && C2.isNegative) {<br></i></div><div><i>              C1 = -C1;<br>
</i></div><div><i>              C2 = -C2;<br>        }<br></i></div><div><i>        int LgC1 = C1.logBase2(),<br>
        int LgC2 = C2.logBase2();<br>
      if (LgC1 == LgC2)<br>
        return "i1 0";<br>
      assert(LgC2 > LgC1);<br>
      // This subtraction takes the place of your original division.<br>
      int Shift = LgC2 - LgC1;<br>
      if (C1.shl(Shift) == C2)<br>
        return "icmp eq A, {Shift}";<br>
      return "i1 0";<br>
   }</i><br><br></div><div>This would avoid division operation. So, basically, we need to calculate shift <br>by LgC2-LgC1 instead of Lg(C2/C1) and compare accordingly :)<br><br></div><div>This can be modified slightly to check 'non-exact' cases as well :<br>
<br><i>// Use lshr here, since we've canonicalized to +ve numbers.<br>
      int Shift = LgC2 - LgC1;<br>
      if (C1 == C2.lshr(Shift))<br>
        return "icmp eq A, {Shift}";<br>
      return "i1 0";<br>
    }</i><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
================<br>
Comment at: test/Transforms/InstCombine/icmp.ll:1427<br>
@@ -1426,1 +1426,3 @@<br>
 }<br>
+<br>
+; CHECK-LABEL: @exact_ashr_eq<br>
----------------<br>
The bugs from last time were due to icmp non-equality comparisons, right? Are those tests already in this code or should you be adding tests to make sure you don't miscompile those cases here?<br>
<br>
<a href="http://reviews.llvm.org/D4068" target="_blank">http://reviews.llvm.org/D4068</a><br></blockquote><div><br></div><div>For now, i am checking only for icmp eq/ne comparisons by adding I.isEquality().<br><br></div>
<div>Other comparisons are TODO item for now. I have made few observations regarding other icmp comparisons,<br></div><div>but need to verify them.<br><br></div><div>(My sincere apology to both Nick and Duncan in case if anyone missed previous review conversation).<br>
</div><br></div>-- <br>With regards,<br>Suyog Sarda<br>
</div></div>