[X86] Replace action Promote with Custom for operation ISD::SINT_TO_FP

[vrybalov]

Summary

If attribute "use-soft-float"="true" is set then X86ISelLowering.cpp sets
'Promote' action for ISD::SINT_TO_FP operation on type i32.

But 'Promote' action is not proper in this case since lib function
__floatsidf is available for casting from signed int to float type.
Thus Custom action is more suitable here.

If function attribute "use-soft-float"="true" is set then infinite looping
can happen in DAG combining, function visitSINT_TO_FP() replaces SINT_TO_FP
node with UINT_TO_FP node and function combineUIntToFP() replace vice versa in cycle.
The fix prevents it.

[RKSimon]

test/CodeGen/X86/sitofp.ll

3 ↗

(On Diff #142191)

This test needs improving - you need to add proper FileCheck testing, preferably use the update_llc_test_checks script to check all codegen and cleanup the actual test - I'm not convinced you need all this code to demonstrate the sitofp?

[vrybalov]

test/CodeGen/X86/sitofp.ll

3 ↗

(On Diff #142191)

Main purpose of the test is to check llc doesn't hang on compilation. Now it hangs.

[vrybalov]

test/CodeGen/X86/sitofp.ll

3 ↗

(On Diff #142191)

Without "target triple = "i386-unknown-linux-gnu"" the test passes compilation now.

[efriedma]

lib/Target/X86/X86ISelLowering.cpp

238 ↗

(On Diff #142191)

Normally, you'd use "Expand" to indicate this is always a libcall.

[craig.topper]

test/CodeGen/X86/sitofp.ll

3 ↗

(On Diff #142191)

This should be sufficient to hit the bug

target triple = "i386-unknown-linux-gnu"

define double @foo(i16 %foo) #0 {
  %conv = zext i16 %foo to i32
  %conv1 = sitofp i32 %conv to double
  ret double %conv1
}

attributes #0 = { "use-soft-float"="true" }

[craig.topper]

lib/Target/X86/X86ISelLowering.cpp

238 ↗

(On Diff #142191)

I think really the problem is that UINT_TO_FP is defaulting to Legal in this case. So this DAG combine is being allowed to fire. Making SINT_TO_FP Custom stops this, but I think you really want to set UINT_TO_FP to Promote. Or maybe set them both to Expand as Eli suggested. It doesn't necessarily matter because we should be softening the float before we get to legalize ops.

// If the input is a legal type, and SINT_TO_FP is not legal on this target,
// but UINT_TO_FP is legal on this target, try to convert.
if (!TLI.isOperationLegalOrCustom(ISD::SINT_TO_FP, OpVT) &&
    TLI.isOperationLegalOrCustom(ISD::UINT_TO_FP, OpVT)) {
  // If the sign bit is known to be zero, we can change this to UINT_TO_FP.
  if (DAG.SignBitIsZero(N0))
    return DAG.getNode(ISD::UINT_TO_FP, SDLoc(N), VT, N0);
}

[vrybalov]

Updated test.

[spatel]

test/CodeGen/X86/sitofp.ll

3 ↗

(On Diff #142191)

The general rule is that if you're going to test something, then you might as well test that the output is *correct* rather than *not wrong*. The script that Simon mentioned makes that easy, especially now that the test is minimized. Please use FileCheck and the script for this test.

[vrybalov]

The patch is updated according to review comments.

[craig.topper]

LGTM

[lebedev.ri]

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

17

It would be really nice for all the tests to be committed first,
so that the following codechange commit actually shows the test change.

[vrybalov]

Simon, thank you for the test patching. I did not check it with recent patches. In the future I will be more careful with it.

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

17

It was impossible to commit the test before the patch. LLC hung on the test and did not produce any output.

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

if (Subtarget.is64Bit()) {
     setOperationAction(ISD::UINT_TO_FP     , MVT::i64  , Custom);
   } else if (!Subtarget.useSoftFloat()) {
     // We have an algorithm for SSE2->double, and we turn this into a
     // 64-bit FILD followed by conditional FADD for other targets.
     setOperationAction(ISD::UINT_TO_FP     , MVT::i64  , Custom);
     // We have an algorithm for SSE2, and we turn this into a 64-bit
     // FILD or VCVTUSI2SS/SD for other targets.
     setOperationAction(ISD::UINT_TO_FP     , MVT::i32  , Custom);
+  } else {
+    setOperationAction(ISD::UINT_TO_FP     , MVT::i32  , Expand);
   }
 
   // Promote i1/i8 SINT_TO_FP to larger SINT_TO_FP's, as X86 doesn't have
   // this operation.
   setOperationAction(ISD::SINT_TO_FP       , MVT::i1   , Promote);
   setOperationAction(ISD::SINT_TO_FP       , MVT::i8   , Promote);
 
   if (!Subtarget.useSoftFloat()) {
     // SSE has no i16 to fp conversion, only i32.
     if (X86ScalarSSEf32) {
       setOperationAction(ISD::SINT_TO_FP     , MVT::i16  , Promote);
       // f32 and f64 cases are Legal, f80 case is not
       setOperationAction(ISD::SINT_TO_FP     , MVT::i32  , Custom);
     } else {
       setOperationAction(ISD::SINT_TO_FP     , MVT::i16  , Custom);
       setOperationAction(ISD::SINT_TO_FP     , MVT::i32  , Custom);
     }
   } else {
     setOperationAction(ISD::SINT_TO_FP     , MVT::i16  , Promote);
-    setOperationAction(ISD::SINT_TO_FP     , MVT::i32  , Promote);
+    setOperationAction(ISD::SINT_TO_FP     , MVT::i32  , Expand);
   }
 
   // Promote i1/i8 FP_TO_SINT to larger FP_TO_SINTS's, as X86 doesn't have
   // this operation.
   setOperationAction(ISD::FP_TO_SINT       , MVT::i1   , Promote);
   setOperationAction(ISD::FP_TO_SINT       , MVT::i8   , Promote);
 
   if (!Subtarget.useSoftFloat()) {

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

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s | FileCheck %s
+
+target triple = "i386-unknown-linux-gnu"
+
+define double @foo(i16 %a) #0 {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    subl $12, %esp
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    movzwl {{[0-9]+}}(%esp), %eax
+; CHECK-NEXT:    movl %eax, (%esp)
+; CHECK-NEXT:    calll __floatsidf
+; CHECK-NEXT:    addl $12, %esp
+; CHECK-NEXT:    retl
+  %conv = zext i16 %a to i32
+  %conv1 = sitofp i32 %conv to double
+  ret double %conv1
+}
+
+attributes #0 = { "use-soft-float"="true" }