[LegalizeTypes] Fix saturation bug for smul.fix.sat

[bjope]

Summary

Make sure we use SETGE instead of SETGT when checking
if the sign bit is zero at SMULFIXSAT expansion.

The faulty expansion occured when doing "expand" of
SMULFIXSAT and the scale was exactly matching the
size of the smaller type. For example doing

i64 Z = SMULFIXSAT X, Y, 32

and expanding X/Y/Z into using two i32 values.

The problem was that we sometimes did not saturate
to min when overflowing.

Here is an example using Q3.4 numbers:

Consider that we are multiplying X and Y.

X = 0x80 (-8.0 as Q3.4)
Y = 0x20 (2.0 as Q3.4)

To avoid loss of precision we do a widening
multiplication, getting a 16 bit result

Z = 0xF000 (-16.0 as Q7.8)

To detect negative overflow we should check if
the five most significant bits in Z are less than -1.
Assume that we name the 4 most significant bits
as HH and the next 4 bits as HL. Then we can do the
check by examining if
(HH < -1) or (HH == -1 && "sign bit in HL is zero").

The fault was that we have been doing the check as
(HH < -1) or (HH == -1 && HL > 0)
instead of
(HH < -1) or (HH == -1 && HL >= 0).

In our example HH is -1 and HL is 0, so the old
code did not trigger saturation and simply truncated
the result to 0x00 (0.0). With the bugfix we instead
detect that we should saturate to min, and the result
will be set to 0x80 (-8.0).

[leonardchan]

LGTM. Good catch

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

if (Saturating) {
                            DAG.getNode(ISD::AND, dl, BoolNVT, HHNeg1, HLNeg));
     }
   } else if (Scale == NVTSize) {
     // If the scales are equal, Lo and Hi are ResultLH and Result HL,
     // respectively. Avoid shifting to prevent undefined behavior.
     Lo = ResultLH;
     Hi = ResultHL;
 
-    // We overflow max if HH > 0 or HH == 0 && HL sign is negative.
-    // We overflow min if HH < -1 or HH == -1 && HL sign is 0.
+    // We overflow max if HH > 0 or HH == 0 && HL sign bit is 1.
+    // We overflow min if HH < -1 or HH == -1 && HL sign bit is 0.
     if (Saturating) {
       SDValue HHPos = DAG.getSetCC(dl, BoolNVT, ResultHH, NVTZero, ISD::SETGT);
       SDValue HHZero = DAG.getSetCC(dl, BoolNVT, ResultHH, NVTZero, ISD::SETEQ);
       SDValue HLNeg = DAG.getSetCC(dl, BoolNVT, ResultHL, NVTZero, ISD::SETLT);
       SatMax = DAG.getNode(ISD::OR, dl, BoolNVT, HHPos,
                            DAG.getNode(ISD::AND, dl, BoolNVT, HHZero, HLNeg));
 
       SDValue HHNeg = DAG.getSetCC(dl, BoolNVT, ResultHH, NVTNeg1, ISD::SETLT);
       SDValue HHNeg1 = DAG.getSetCC(dl, BoolNVT, ResultHH, NVTNeg1, ISD::SETEQ);
-      SDValue HLPos = DAG.getSetCC(dl, BoolNVT, ResultHL, NVTZero, ISD::SETGT);
+      SDValue HLPos = DAG.getSetCC(dl, BoolNVT, ResultHL, NVTZero, ISD::SETGE);
       SatMin = DAG.getNode(ISD::OR, dl, BoolNVT, HHNeg,
                            DAG.getNode(ISD::AND, dl, BoolNVT, HHNeg1, HLPos));
     }
   } else if (Scale < VTSize) {
     // If the scale is instead less than the old VT size, but greater than or
     // equal to the expanded VT size, the first part of the result (ResultLL) is
     // no longer a part of Lo because it would be scaled out anyway. Instead we
     // can start shifting right from the fourth part (ResultHH) to the second

llvm/trunk/test/CodeGen/X86/smul_fix_sat.ll

 ; X86-NEXT:    movl %ecx, %edx
 ; X86-NEXT:    subl {{[0-9]+}}(%esp), %edx
 ; X86-NEXT:    movl %esi, %edi
 ; X86-NEXT:    sbbl $0, %edi
 ; X86-NEXT:    cmpl $0, {{[0-9]+}}(%esp)
 ; X86-NEXT:    cmovnsl %esi, %edi
 ; X86-NEXT:    cmovnsl %ecx, %edx
 ; X86-NEXT:    testl %edx, %edx
-; X86-NEXT:    setg %cl
+; X86-NEXT:    setns %cl
 ; X86-NEXT:    sets %ch
 ; X86-NEXT:    testl %edi, %edi
 ; X86-NEXT:    setg %bl
 ; X86-NEXT:    sete %bh
 ; X86-NEXT:    andb %ch, %bh
 ; X86-NEXT:    orb %bl, %bh
 ; X86-NEXT:    movl $2147483647, %esi # imm = 0x7FFFFFFF
 ; X86-NEXT:    cmovnel %esi, %edx