[PATCH] D64331: [LegalizeTypes] Fix saturation bug for smul.fix.sat

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope created this revision.
bjope added reviewers: leonardchan, bevinh.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64331

Files:
  llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
  llvm/test/CodeGen/X86/smul_fix_sat.ll


Index: llvm/test/CodeGen/X86/smul_fix_sat.ll
===================================================================
--- llvm/test/CodeGen/X86/smul_fix_sat.ll
+++ llvm/test/CodeGen/X86/smul_fix_sat.ll
@@ -629,7 +629,7 @@
 ; 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
Index: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
+++ llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
@@ -2902,8 +2902,8 @@
     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);
@@ -2913,7 +2913,7 @@
 
       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));
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D64331.208394.patch
Type: text/x-patch
Size: 1712 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190708/12f0a6e3/attachment.bin>