<div dir="ltr"><div>Hi Duncan,<br><br></div>Thanks for your review :)<br><br><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
</div></div>Hi Suyog,<br>
<br>
Thanks for continuing to work on this.<br>
<br>
I saw your reply about certain simplifications being handled elsewhere,<br>
but I'm finding the assumptions in the code difficult to follow. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Firstly, I think you should handle every case locally. There are two<br>
ways to do this: with `assert`s for cases you can guarantee are handled<br>
elsewhere, and with control flow for everything else. IMO, all the<br>
cases are so straightforward that there's no downside to just repeating<br>
the simplification work.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
As is, you're relying on transformations elsewhere to make this code<br>
correct. It's nearly impossible to confirm that the transformation is<br>
logically sound, and what happens if someone removes (or disables) the<br>
other transformation(s)?<br></blockquote><div><br><div>Actually, i have omitted the code for test cases which never reaches this place.<br></div><div>All such omitted cases are handled in the 'SimplifyICmpInst' call which is just at the start<br>
</div><div>of 'VisitICmp' function. I ran 'instsimplify' pass on such cases and verified. <br><br>Since, for every 'Combine' function, we visit 'Simplify' first, i didn't include the code for it. <br>
If those 'Simplify' calls at the start are removed (in future may be), <br>then we will have to handle those cases in 'Combine'.<br></div><div><br></div>'Assert' seems a good option. But writing code for all the cases will improve readability and flow of logic, <br>
though may be redundant.<br><br></div><div>Can you please suggest, what should i select? <br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Secondly, you're handling both `lshr` and `ashr`, which I think you can<br>
do if you add the right logic -- but you're handling them identically.<br>
This seems unlikely to be correct.<br>
<br>
Thirdly, it looks like you're matching both `exact` and non-`exact`<br>
cases. This is possible -- with `exact`, the result is a poison value,<br>
so you have some freedom -- but your comments only talk about `exact`,<br>
and it's not clear that the code here is correct otherwise.<br></blockquote><div><br></div><div>Since, the special cases for exact were handled in 'instsimplify' for lshr and ashr, <br></div><div>i combined the common logic. The only difference was checking the equality after <br>
calculating the logs, which i have handled as separate cases.<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>
I have some more specific comments below.<br>
<div><br>
Index: lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
===================================================================<br>
--- lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
+++ lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
</div>@@ -2459,6 +2459,48 @@<br>
<div> return new ICmpInst(I.getPredicate(), A, B);<br>
}<br>
<br>
+ // PR19753:<br>
</div>+ // (icmp eq/ne (ashr/lshr exact const2, A), const1) -> (icmp eq/ne A,<br>
+ // Log2(const2/const1)) -> (icmp eq/ne A, Log2(const2) - Log2(const1)).<br>
<br>
This would read better like this:</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
// (icmp eq/ne (ashr/lshr exact const2, A), const1) -><br>
// (icmp eq/ne A, Log2(const2/const1)) -><br>
// (icmp eq/ne A, Log2(const2) - Log2(const1)).<br></blockquote><div><br>Will rectify this :)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
+ // TODO : Handle this for other icmp instructions.<br>
</div>+ if (I.isEquality()) {<br>
+ ConstantInt *CI2;<br>
+ if (match(Op0, m_AShr(m_ConstantInt(CI2), m_Value(A))) ||<br>
+ match(Op0, m_LShr(m_ConstantInt(CI2), m_Value(A)))) {<br>
<br>
Shouldn't this be checking for `exact`?<br></blockquote><div><br>The only difference for 'exact' and 'non-exact' was checking the equality after <br>calculating the logs, which i have handled as separate cases below.<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
+ APInt AP1 = CI->getValue();<br>
+ APInt AP2 = CI2->getValue();<br>
+ int Shift;<br>
+ // (icmp eq/ne (ashr/lshr exact const2, A), 0) -><br>
+ // (icmp ugt A, Log2(const2)).<br>
+ // const2 = 0/ both const = 0 is already handled previously.<br>
<br>
Is this guaranteed, even if `-instcombine` is run on its own? Is that<br>
simplification guaranteed to run before this one? If you're sure you<br>
can guarantee this, you should use an `assert`:<br>
<br>
assert(AP2 && "Shift by 0 should be simplified by now"); <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
If not, you should handle it explicitly.<br></blockquote><div><br><div>As mentioned earlier, this is handled in 'Simplify'.<br></div>I will modify the code (assert or handle the case itself), as per you suggestion. <br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
+ if (!AP1) {<br>
+ Shift = AP2.logBase2();<br>
+ return new ICmpInst(I.ICMP_UGT, A,<br>
+ ConstantInt::get(A->getType(), Shift));<br>
+ }<br>
+ // if const1 = const2 -> icmp eq/ne A, 0<br>
+ if (CI == CI2)<br>
<div>+ return new ICmpInst(I.getPredicate(), A,<br>
</div>+ ConstantInt::getNullValue(A->getType()));<br>
+ // If both the constants are negative, take their positive to calculate<br>
+ // log.<br>
+ if (AP1.isNegative() || AP2.isNegative()) {<br>
+ AP1 = -AP1;<br>
+ AP2 = -AP2;<br>
+ }<br>
<br>
This logic reads strangely, and I think it's wrong for `lshr`.<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
If you can guarantee that they are both negative or neither negative,<br>
then this might be reasonable:<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
if (isa<AShrOperator>(Op0) {<br>
assert(AP1.isNegative() == AP2.isNegative() &&<br>
"Different signs should be simplified by now");<br>
if (AP1.isNegative()) {<br>
AP1 = -AP1;<br>
AP2 = -AP2;<br>
}<br>
}<br>
<br>
If you can't guarantee it, you should handle it explicitly instead.<br></blockquote><div><br><div>There should be '&&' instead of '||'. i will rectify it. <br></div> <br>For, both ashr/lshr "exact" if the constants have opposite sign, <br>
</div><div>'Simplify' returns false, and we never reach this place. <br></div><div><br>For ashr non-'exact', if the constants have opposite sign, 'Simplify'<br></div><div>returns false. <br><br>For lshr non-'exact', if const2 is positive and const1 is negative,<br>
</div><div>'Simplify' returns false. <br><br></div><div>For lshr non-'exact', if const2 is negative and const1 is positive,<br></div><div>we will have to handle this case. I missed this, will handle it.<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>
+ Shift = AP2.logBase2() - AP1.logBase2();<br>
+ if ((cast<BinaryOperator>(Op0)->isExact()) && (AP1.shl(Shift) == AP2))<br>
<div>+ return new ICmpInst(I.getPredicate(), A,<br>
</div>+ ConstantInt::get(A->getType(), Shift));<br>
+ // Use lshr here, since we've canonicalized to +ve numbers.<br>
+ else if (AP1 == AP2.lshr(Shift))<br>
<br>
Remove the `else`. This should simply be:<br>
<br>
if (AP1 == AP2.lshr(Shift))<br>
<div><br>
+ return new ICmpInst(I.getPredicate(), A,<br>
</div>+ ConstantInt::get(A->getType(), Shift));<br>
+ else<br>
<br>
Remove the `else` and un-indent the following `return`.<br>
<br>
+ return ReplaceInstUsesWith(I, ConstantInt::getFalse(I.getType()));<br>
<div>+ }<br>
+ }<br>
+<br>
// If we have an icmp le or icmp ge instruction, turn it into the<br>
// appropriate icmp lt or icmp gt instruction. This allows us to rely on<br>
// them being folded in the code below. The SimplifyICmpInst code has
</div></blockquote></div><br></div><div class="gmail_extra">I will properly re-factor the code, once i get suggestion from you, whether to use assert or to simply<br></div><div class="gmail_extra">include all the cases even if they are handled previously.<br>
<br></div><div class="gmail_extra">( Now, i am also inclined towards including all cases for simplicity in logic flow. <br>I had not included earlier it just to avoid redundancy) <br></div><div class="gmail_extra">
<br></div><div class="gmail_extra">Awaiting for your suggestion :)<br></div><div class="gmail_extra"><br clear="all"><br>-- <br>With regards,<br>Suyog Sarda<br>
</div></div>