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

[Frits van Bommel]

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
>>
>> Why only for sext and not zext? Does scalar evolution understand those
>> well enough that it's not needed there or was it just not useful for
>> your particular test case?
>> Or maybe the zext case is too different from the sext case to add easily?
>
> I wanted to check first if the approach I took was OK. The new version
> contains also zext support.


> Attached is the new version.

       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.

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


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


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.

The same doesn't hold for zext + signed compare, by the way. For
instance, 0xff <s 0x00, but 0x00ff >s 0x0000.