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

suyog suyog.sarda at samsung.com
Fri May 30 02:20:54 PDT 2014


Hi Rafael, 
Thanks a lot for the review.

> 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.

>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
}

> 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
}

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? 

> 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. 

Thanks.

http://reviews.llvm.org/D3959






More information about the llvm-commits mailing list