[llvm-commits] [PATCH] Fix: instcombine does not get max if hidden by sext

[Tobias Grosser]

On 01/07/2011 11:01 AM, Frits van Bommel wrote:
> On Fri, Jan 7, 2011 at 3:58 PM, Tobias Grosser
> <grosser at fim.uni-passau.de>  wrote:
>> On 01/06/2011 05:49 AM, Frits van Bommel wrote:
>>>
>>> On Thu, Jan 6, 2011 at 7:49 AM, Chris Lattner<clattner at apple.com>    wrote:
>>>>
>>>> On Jan 3, 2011, at 8:52 PM, Tobias Grosser wrote:
>>>>>
>>>>> I fixed a recent bug report, that blocked me on a large FORTRAN test
>>>>> case:
>>>>>
>>>>>     InstCombine:
>>>>>     X = sext x ; x<    c ? X : C-1
>>>>>     -->
>>>>>     X = sext x; X>    C-1 ? C-1 : X
>>>>>
>>>>>     Instead of calculating this with mixed types promote all to the
>>>>>     larger type. This enables scalar evolution to analyze this
>>>>>     expression. PR8866
>         switch (Pred) {
>         default: break;
> -      case ICmpInst::ICMP_ULT:
>         case ICmpInst::ICMP_SLT: {
>           // X<  MIN ? T : F  -->   F
> -        if (CI->isMinValue(Pred == ICmpInst::ICMP_SLT))
> +        if (CI->isMinValue(true /* signed */))
>             return ReplaceInstUsesWith(SI, FalseVal);
> ...
>           break;
>         }
> -      case ICmpInst::ICMP_UGT:
>         case ICmpInst::ICMP_SGT: {
>           // X>  MAX ? T : F  -->   F
> -        if (CI->isMaxValue(Pred == ICmpInst::ICMP_SGT))
> +        if (CI->isMaxValue(true /* signed */))
>             return ReplaceInstUsesWith(SI, FalseVal);
> +        break;
> +      }
>
> Why not do this for the unsigned cases anymore? X<u 0 is known false,
> as is X>u -1.
Fixed.

> Also, this switch might be cleaner as an if/else if but that's perhaps
> a matter of taste.
Fixed.

> +          // X = sext x; x>s c ? X : C+1 -->  X = sext x; X<s C+1 ? C+1 : X
> +          // X = sext x; x<s c ? X : C-1 -->  X = sext x; X>s C-1 ? C-1 : X
> +          if (Pred == ICI->getSignedPredicate()) {
>
> You can use ICI->isSigned() or (I)CmpInst::isSigned(Pred) here.
Fixed.

> I also noticed that you're no longer handling the sext + unsigned
> compare case. I know it's kind of counter-intuitive, but when I was
> examining the original patch I tried to find a case where it was wrong
> but couldn't find one.
> For instance, for x<u y:
> If both x and y are<s 0 or both are>=s 0, (sext x) and (sext y) are
> still the same relative values when interpreted as unsigned (just with
> possibly some 1s prepended) so the result of x<u y doesn't change by
> sext'ing the operands.
> If x<s 0 and y>=s 0, x<u y is false and prepending 1s to x and 0s
> to y doesn't change that.
> If x>=s 0 and y<s 0, x<u y is true and prepending 0s to x and 1s to
> y doesn't change that.
> A similar argument can be made for x>u y.
I added support for this case and two more test cases.

> The same doesn't hold for zext + signed compare, by the way. For
> instance, 0xff<s 0x00, but 0x00ff>s 0x0000.
I copied this example in a comment.

Thanks for your helpful review. New version attached.

Cheers
Tobi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-InstCombine-Match-min-max-hidden-by-sext-zext.patch
Type: text/x-diff
Size: 6883 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110107/dfb45f2f/attachment.patch>