<div dir="ltr"><div>+ llvm-commit (Sorry, i forgot to add while replying earlier)</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jul 4, 2014 at 4:14 PM, suyog sarda <span dir="ltr"><<a href="mailto:sardask01@gmail.com" target="_blank">sardask01@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hi Duncan,<br><br></div>Thanks a lot for the review. My comments inline :<br><div><div><div class="gmail_extra">
<br><br><div class="gmail_quote"><div class="">On Thu, Jul 3, 2014 at 8:28 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</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"><div><br>
> On 2014 Jul 3, at 00:07, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
>> On 2014 Jul 2, at 14:33, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:<br>
>><br>
>> Sorry for the delay. LGTM...but considering that versions of this patch have caused miscompiles twice and I'm pretty new here, I think we should have someone with more instcombine experience give final approval in case I've missed anything.<br>


>><br>
>> <a href="http://reviews.llvm.org/D4068" target="_blank">http://reviews.llvm.org/D4068</a><br>
>><br>
><br>
> Hi Suyog,<br>
><br>
> It seems like you might be able to go a little further without much<br>
> work.<br>
<br>
</div>After thinking about it overnight, I think if you reverse the order of<br>
the lg and divide, you'll avoid the expensive division.  As a bonus, it<br>
looks to me like you can always combine the instructions if you do this<br>
carefully.<br>
<br>
I.e., here's some pseudo-code for `ashr`:<br>
<br>
    // (icmp eq/ne (ashr exact C2, A), C1)<br>
    APInt C1, C2;<br>
    if (matchesAShrExact(..., C1, C2)) {<br>
      if (!C1) {<br>
        if (!C2)<br>
          return "i1 1";<br>
        return "i1 0";<br>
      }<br></blockquote><div><br></div></div><div>I believe this case is handled earlier in the code (maybe 'instsimplify') as observed below.<br></div><div>e.g:<br><br>cat test.ll<br>define i1 @main(i32 %a) {<br>
  %shr = ashr exact i32 0, %a<br>
  %cmp = icmp eq i32 %shr, 0  <br>  ret i1 %cmp<br>}<br><br></div><div>Output without my patch: <br></div><div> llvm/llvm/build/bin/opt -S -instcombine test.ll<br><br>define i1 @main(i32 %a) {<br>  ret i1 true<br>}<br><br>

e.g:<br><br>define i1 @main(i32 %a) {<br>  %shr = ashr exact i32 -80, %a<br>  %cmp = icmp eq i32 %shr, 0  <br>  ret i1 %cmp<br>}<br><br></div><div>Output without my patch :<br><br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br>

<br>define i1 @main(i32 %a) {<br>  ret i1 false<br>}<br><br></div><div>Same for lshr.<br><br></div><div class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

      if (!C2)<br>
        return "i1 0";<br></blockquote><div><br></div></div><div>This is also handled earlier :<br></div><div>e.g:<br><br>define i1 @main(i32 %a) {<br>  %shr = ashr exact i32 0, %a<br>  %cmp = icmp eq i32 %shr, -21  <br>

  ret i1 %cmp<br>}<br><br>Output without my patch:<br><br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br><br>define i1 @main(i32 %a) {<br>  ret i1 false<br>}<br></div><div><br></div><div>Same for lshr.<br></div><div class="">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
      if (C1 == C2)<br>
        return "icmp eq A, 0";<br></blockquote><div><br></div></div><div>This is also handled. Though output seems weird. <br><br></div><div>e.g:<br><br>define i1 @main(i32 %a) {<br>  %shr = ashr exact i32 -81, %a<br>

  %cmp = icmp eq i32 %shr, -81  <br>  ret i1 %cmp<br>}<br><br></div><div>Output without my patch:<br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br><br>define i1 @main(i32 %a) {<br>  ret i1 true<br>}<br><br></div><div>

e.g:<br><br>define i1 @main(i32 %a) {<br>  %shr = lshr exact i32 -81, %a<br>  %cmp = icmp eq i32 %shr, -81  <br>  ret i1 %cmp<br>}<br><br>Output without my patch:<br><br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br>

<br>define i1 @main(i32 %a) {<br>  ret i1 true<br>}<br><br></div><div>e.g:<br><br>define i1 @main(i32 %a) {<br>  %shr = lshr exact i32 81, %a<br>  %cmp = icmp eq i32 %shr, 81  <br>  ret i1 %cmp<br>}<br><br>Output without my patch:<br>

<br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br><br>define i1 @main(i32 %a) {<br>  ret i1 true<br>}<br><br></div><div>e.g:<br><br>define i1 @main(i32 %a) {<br>  %shr = ashr exact i32 81, %a<br>  %cmp = icmp eq i32 %shr, 81  <br>

  ret i1 %cmp<br>}<br><br></div><div>Output : No change. Same code as above.<br></div><div class=""><div><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">


      if (C1.isNegative() != C2.isNegative())<br>
        return "i1 0";<br></blockquote><div><br></div></div><div>I believe this case is for one of the constant being positive and one being negative. <br></div><div>This is also handled.<br><br></div><div>e.g:<br>
<br>
define i1 @main(i32 %a) {<br>  %shr = ashr exact i32 -80, %a<div class=""><br>  %cmp = icmp eq i32 %shr, 41  <br></div>  ret i1 %cmp<br>}<br><br></div><div>Output without my patch:<br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br>
<br>define i1 @main(i32 %a) {<br>
  ret i1 false<br>}<br><br></div><div>Same for lshr.<br></div><div class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
      if (C1.isNegative()) {<br>
        C1 = -C1;<br>
        C2 = -C2;<br>
      }<br></blockquote><div><br></div></div><div>At this point, both the constants are negative.<br></div><div class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


      if (C1 > C2)<br>
        return "i1 0";<br></blockquote><div><br></div></div><div>This is also handled.<br><br></div><div>e.g:<br><br>define i1 @main(i32 %a) {<br>  %shr = ashr exact i32 -40, %a<br>  %cmp = icmp eq i32 %shr, -81  <br>

  ret i1 %cmp<br>}<br><br></div><div>Output without my patch:<br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br><br>define i1 @main(i32 %a) {<br>  ret i1 false<br>}<br><br></div><div>e.g:<br><br>define i1 @main(i32 %a) {<div class="">
<br>
  %shr = lshr exact i32 40, %a<br></div>  %cmp = icmp eq i32 %shr, 81  <br>  ret i1 %cmp<br>}<br><br></div><div>Output without my patch:<br><br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br><br>define i1 @main(i32 %a) {<br>

  ret i1 false<br>}<br><br></div><div>e.g:<br>define i1 @main(i32 %a) {<br>  %shr = lshr exact i32 -40, %a<br>  %cmp = icmp eq i32 %shr, -81  <br>  ret i1 %cmp<br>}<br><br></div><div>Output: No change in above code<br></div>
<div class="">
<div><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">
      int LgC1 = C1.logBase2(),<br>
          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>
   }<br>
<br>
I think other than the handling of `isNegative()`, the same logic works<br>
for `lshr`.<br></blockquote><div><br></div></div><div>Yes, i agree this is more cost effective way to calculate shift. I will update the patch<br></div><div>to use this instead of division.<br></div><div class=""><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>
I think you can use the same basic logic for non-`exact` right-shifts as<br>
well.  I believe you just need to modify the initial 0-related checks<br>
and reverse the direction of the shift, effectively masking out the<br>
lower bits of C2 in the comparison at the end:<br>
<br>
    // (icmp eq/ne (ashr C2, A), C1)<br>
    APInt C1, C2;<br>
    if (matchesAShrNotExact(..., C1, C2)) {<br>
      if (!C1) {<br>
        // Corner cases for C1 == 0 are different.<br>
        if (!C2)<br>
          return "i1 1";<br></blockquote></div><div><br>This is handled earlier.<br></div><div class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


        if (C2.isNegative())<br>
          return "i1 0";<br></blockquote><div><br></div></div><div>This is also handled earlier.<br></div><div class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


        int Shift = C2.logBase2();<br>
        return "icmp gt A, {Shift};<br>
      }<br></blockquote><div><br></div></div><div>This is not handled. Will include it in my patch.<br></div><div class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


      if (!C2)<br>
        return "i1 0"; </blockquote><div><br></div></div><div>This is handled earlier.<br></div><div class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">


      // ... same code as before, until:<br>
      // 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>
    }<br></blockquote><div><br></div></div><div>This i will include in my patch.<br></div><div class=""><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>
Again, I think that aside from negative checking, the same basic logic<br>
works for `lshr`.<br></blockquote><div><br><br><br></div></div><div>So, basically, we need to calculate shift by LgC2-LgC1 instead of Lg(C2/C1)<br></div><div>and compare accordingly :)<br><br></div><div>I will soon update the patch.<br>

<br></div><div>Thanks!!<span class="HOEnZb"><font color="#888888"><br></font></span></div></div><span class="HOEnZb"><font color="#888888"><br>-- <br>With regards,<br>Suyog Sarda<br>
</font></span></div></div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>With regards,<br>Suyog Sarda<br>
</div>