<div dir="ltr"><div>Thanks, Eli. I made a couple of follow-on cleanups before I saw your mail.<br></div>Draft posted for review - make sure the comments don't cause more confusion than they remove. :) <br><a href="https://reviews.llvm.org/D23819" target="_blank">https://reviews.llvm.org/<wbr>D23819</a><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 23, 2016 at 3:36 PM, Friedman, Eli <span dir="ltr"><<a href="mailto:efriedma@codeaurora.org" target="_blank">efriedma@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 8/23/2016 2:01 PM, Sanjay Patel via llvm-commits wrote:<br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
Author: spatel<br>
Date: Tue Aug 23 16:01:35 2016<br>
New Revision: 279568<br>
<br></span><span class="">
-  // If we are comparing against bits always shifted out, the<br>
-  // comparison cannot succeed.<br>
-  APInt Comp = CmpRHSV << ShAmtVal;<br>
-  ConstantInt *ShiftedCmpRHS = Builder->getInt(Comp);<br>
-  if (Shr->getOpcode() == Instruction::LShr)<br>
-    Comp = Comp.lshr(ShAmtVal);<br>
-  else<br>
-    Comp = Comp.ashr(ShAmtVal);<br>
-<br>
-  if (Comp != CmpRHSV) { // Comparing against a bit that we know is zero.<br>
-    bool IsICMP_NE = ICI.getPredicate() == ICmpInst::ICMP_NE;<br>
-    Constant *Cst = Builder->getInt1(IsICMP_NE);<br>
-    return replaceInstUsesWith(ICI, Cst);<br>
-  }<br>
-<br>
-  // Otherwise, check to see if the bits shifted out are known to be zero.<br>
-  // If so, we can compare against the unshifted value:<br>
+  // Check if the bits shifted out are known to be zero. If so, we can compare<br>
+  // against the unshifted value:<br>
</span></blockquote>
<br>
It's not obvious that it's actually safe to do this... I mean, you don't need the same optimization in both InstSimplify and Instcombine, but "Comp != CmpRHSV" also acts as a safety check here.  At the very least, there needs to be a comment explaining why this is safe.<br>
<br>
-Eli<div class="HOEnZb"><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
    //  (X & 4) >> 1 == 2  --> (X & 4) == 4.<br>
+  ConstantInt *ShiftedCmpRHS = Builder->getInt(CmpRHSV << ShAmtVal);<br>
    if (Shr->hasOneUse() && Shr->isExact())<br>
      return new ICmpInst(ICI.getPredicate(), Shr->getOperand(0), ShiftedCmpRHS);<br>
  <br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
<br></div></div><span class="HOEnZb"><font color="#888888">
-- <br>
Employee of Qualcomm Innovation Center, Inc.<br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project<br>
<br>
</font></span></blockquote></div><br></div>