[PATCH] Patch for PR19753 InstCombine: constant comparison involving "ashr exact" not optimized well

[Rafael Ávila de Espíndola]

>> const1 in the comment is CI2 and const2 is CI, which is a bit confusing :-)
> :) Actually, CI already exist in the code, hence i named the new constant as CI2. I can change the comment or change the variable name from CI2 to CI1. Let me know what is good.

Just the comment is fine.

>>If const2 doesn't exactly divide const1 I think we can produce a
>>undef, which is probably more profitable, right?
>
> Yups. But interestingly, there is a call to 'SimplifyICmpInst' before my code, which figures out if there is exact division or not.
> If there is no exact division, then it returns false and it never hits my code.
>
> e.g
> define i1 @exact_ashr_eq_false(i32 %a) {
>   %shr = ashr exact i32 -34, %a
>   %cmp = icmp eq i32 %shr, -15
>   ret i1 %cmp
> }
>
> (-34 doesn't divide -15 exactly)
>
> Output without my patch:
>
> define i1 @exact_ashr_eq_false(i32 %a) {
>   ret i1 false
> }
>
> Output with my patch:
>
> define i1 @exact_ashr_eq_false(i32 %a) {
>   ret i1 false
> }

I see, thanks.

>> Some thing if the log is not exact. That means that the exact flag in
>> the ashr was wrong.
>
> This is also handled by 'SimplifyICmpInst' call.
>
> e.g
> define i1 @exact_ashr_eq_false(i32 %a) {
>   %shr = ashr exact i32 -90, %a
>   %cmp = icmp eq i32 %shr, -15
>   ret i1 %cmp
> }
>
> (-15 divides -90 exactly but log2(-90/-15) > log2(6) is not exact)
>
> Output without my patch:
>
> define i1 @exact_ashr_eq_false(i32 %a) {
>   ret i1 false
> }
>
> Output with my patch:
>
> define i1 @exact_ashr_eq_false(i32 %a) {
>   ret i1 false
> }

Awesome :-)

> So for both the cases - not exactly divisible and not exact log - it never reaches my code, 'SimplifyICmpInst' call previous to my code takes care of both cases.
> Should i still add the two checks?

No, it is fine. Now that I think of it better, code that can produce
just a constant (like the above) should live in instsimplify, not
instcombine.

>> Could lshr be handled in a similar (udiv instead of sdiv) way if it
>> has an exact flag?
>> Not sure if you want to handle these now or in another patch, but at
>> least add a FIXME comment.
>
> We can handle this also. But i will try to implement it another patch. i will add FIXME/TODO for this.
>
> Please let me know regarding the two checks. I will make the changes accordingly.

LGTM with

* FIXME/TODO about the lhsr
* The comment variable names update in the comment.
* A comment about the inexact div and log being handled in instsimplify

Cheers,
Rafael

http://reviews.llvm.org/D3959