<div dir="ltr">Thanks Duncan for your quick reply :)<br><div class="gmail_extra"><br><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
After looking at `SimplifyICmpInst`, the logic there seems disjoint </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">enough that I think it's better just to handle the cases redundantly<br>
here. Even though it's called directly from this function, it's hard to<br>
verify exactly how its transformations overlap with these ones. Unless<br>
you can show a non-negligible compile time improvement from switching to<br>
`assert`s, I think the clearer code wins.<br>
<br>
(It's a weak preference, so if anyone following along thinks `assert`<br>
is better here, pipe up!)<br></blockquote><div><br></div><div>Seems better to include all the cases. I will update the patch accordingly.<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">
<div><div class="h5"><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>
><br>
> Since, the special cases for exact were handled in 'instsimplify' for lshr and ashr,<br>
> 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>
><br>
><br>
> I have some more specific comments below.<br>
><br>
> Index: lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
> ===================================================================<br>
> --- lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
> +++ lib/Transforms/InstCombine/InstCombineCompares.cpp<br>
> @@ -2459,6 +2459,48 @@<br>
> return new ICmpInst(I.getPredicate(), A, B);<br>
> }<br>
><br>
> + // PR19753:<br>
> + // (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:<br>
><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>
><br>
> Will rectify this :)<br>
><br>
><br>
> + // TODO : Handle this for other icmp instructions.<br>
> + 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>
><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>
><br>
><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>
><br>
> If not, you should handle it explicitly.<br>
><br>
> As mentioned earlier, this is handled in 'Simplify'.<br>
> I will modify the code (assert or handle the case itself), as per you suggestion.<br>
><br>
><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>
> + return new ICmpInst(I.getPredicate(), A,<br>
> + 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>
><br>
> If you can guarantee that they are both negative or neither negative,<br>
> then this might be reasonable:<br>
><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>
><br>
> There should be '&&' instead of '||'. i will rectify it.<br>
><br>
> For, both ashr/lshr "exact" if the constants have opposite sign,<br>
> 'Simplify' returns false, and we never reach this place.<br>
<br>
</div></div>I'm not convinced about `lshr exact`. Consider:<br>
<br>
%Shifted = lshr exact i2 2, %A<br>
%Compare = icmp eq i2 %Shifted, 1<br>
<br>
Here, 2 is negative and 1 is positive, but the %Compare should be `i1 1`<br>
when %A is 1.<br>
<br>
If simplify gives `i1 0` for this, it sounds like a bug to me. Can you<br>
confirm?<br></blockquote><div><br></div><div>Strangely, the output varies here.<br><br>cat test.ll<br>define i1 @main(i2 %a) {<br> %shr = lshr exact i2 2, %a<br> %cmp = icmp eq i2 %shr, 1<br> ret i1 %cmp<br>}<br></div>
<div><br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br>; ModuleID = 'test.ll'<br><br>define i1 @main(i2 %a) {<br> %shr = lshr exact i2 -2, %a<br> %cmp = icmp eq i2 %shr, 1<br> ret i1 %cmp<br>}<br><br>llvm/llvm/build/bin/opt -S -instsimplify test.ll<br>
; ModuleID = 'test.ll'<br><br>define i1 @main(i2 %a) {<br> %shr = lshr exact i2 -2, %a<br> %cmp = icmp eq i2 %shr, 1<br> ret i1 %cmp<br>}<br><br></div><div>As visible above, the 'Simplify' or 'Combine' doesn't return false.<br>
<br></div><div>However,<br></div><div>Consider following Test case :<br><br>cat test.ll<br>define i1 @main(i32 %a) {<br> %shr = lshr exact i32 -64, %a // One positive one negative<br> %cmp = icmp eq i32 %shr, 16<br> ret i1 %cmp<br>
}<br><br>llvm/llvm/build/bin/opt -S -instsimplify test.ll<br>; ModuleID = 'test.ll'<br><br>define i1 @main(i32 %a) {<br> ret i1 false<br>}<br><br>llvm/llvm/build/bin/opt -S -instcombine test.ll<br>; ModuleID = 'test.ll'<br>
<br>define i1 @main(i32 %a) {<br> ret i1 false<br>}<br><br></div><div>It seems, 'Simplify' is behaving differently for 'extreme (MIN/MAX)' values <br></div><div>and values in 'middle'. Is this logically correct (doesn't seem so)? Or a bug?<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">
<div class=""><br>
> For ashr non-'exact', if the constants have opposite sign, 'Simplify'<br>
> returns false.<br>
><br>
> For lshr non-'exact', if const2 is positive and const1 is negative,<br>
> 'Simplify' returns false.<br>
<br>
</div>Adding the logic (or `assert`s) will make this more obvious :).<br>
<div class=""><div class="h5"><br>
> For lshr non-'exact', if const2 is negative and const1 is positive,<br>
> we will have to handle this case. I missed this, will handle it.<br>
><br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br>With regards,<br>Suyog Sarda<br>
</div></div>