[PATCH] PR19958 wrong code at -O1 and above on x86_64-linux-gnu (InstCombine)

[suyog sarda]

Hi Nick,

Thanks a lot for the review :)

I think you missed review conversation with Duncan (It was my mistake that
i forgot
to cc llvm-commit, but later did it. My sincere apology).


On Sun, Jul 6, 2014 at 7:34 AM, Nick Lewycky <nicholas at mxc.ca> wrote:

> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2349
> @@ +2348,3 @@
> +      (cast<BinaryOperator>(Op)->isExact())) {
> +    if (CI2->getValue().srem(CI->getValue()) == 0) {
> +      Quotient = CI2->getValue().sdiv(CI->getValue());
> ----------------
> APInt::isMinValue() is faster than APInt::operator== even though it's less
> clear to read.
>
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2350
> @@ +2349,3 @@
> +    if (CI2->getValue().srem(CI->getValue()) == 0) {
> +      Quotient = CI2->getValue().sdiv(CI->getValue());
> +      return true;
> ----------------
> APInt::sdivrem is as fast as a single sdiv or srem. Please use it then
> text the 'rem' results.
>
>
Duncan has suggested to calculate 'rem' and 'Shift' as following:


*if(C1.isNegative && C2.isNegative) {*

*              C1 = -C1;*


*              C2 = -C2;        }*










*        int LgC1 = C1.logBase2(),         int LgC2 = C2.logBase2();
if (LgC1 == LgC2)         return "i1 0";       assert(LgC2 > LgC1);
// This subtraction takes the place of your original division.       int
Shift = LgC2 - LgC1;       if (C1.shl(Shift) == C2)         return "icmp eq
A, {Shift}";       return "i1 0";    }*

This would avoid division operation. So, basically, we need to calculate
shift
by LgC2-LgC1 instead of Lg(C2/C1) and compare accordingly :)

This can be modified slightly to check 'non-exact' cases as well :






*// Use lshr here, since we've canonicalized to +ve numbers.       int
Shift = LgC2 - LgC1;       if (C1 == C2.lshr(Shift))         return "icmp
eq A, {Shift}";       return "i1 0";     }*


> ================
> Comment at: test/Transforms/InstCombine/icmp.ll:1427
> @@ -1426,1 +1426,3 @@
>  }
> +
> +; CHECK-LABEL: @exact_ashr_eq
> ----------------
> 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?
>
> http://reviews.llvm.org/D4068
>

For now, i am checking only for icmp eq/ne comparisons by adding
I.isEquality().

Other comparisons are TODO item for now. I have made few observations
regarding other icmp comparisons,
but need to verify them.

(My sincere apology to both Nick and Duncan in case if anyone missed
previous review conversation).

-- 
With regards,
Suyog Sarda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140706/fe46fdb5/attachment.html>