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

Authored by vrybalov on Thu, Apr 12, 8:26 AM.

Details

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.

Diff Detail

vrybalov created this revision.Thu, Apr 12, 8:26 AM
vrybalov edited the summary of this revision. (Show Details)Thu, Apr 12, 8:30 AM
vrybalov added a reviewer: ashlykov.
vrybalov edited reviewers, added: craig.topper; removed: ashlykov.Thu, Apr 12, 9:03 AM
vrybalov added a subscriber: ashlykov.
RKSimon added inline comments.
test/CodeGen/X86/sitofp.ll
4

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 added inline comments.Mon, Apr 16, 7:42 AM
test/CodeGen/X86/sitofp.ll
4

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

vrybalov added inline comments.Mon, Apr 16, 7:49 AM
test/CodeGen/X86/sitofp.ll
4

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

efriedma added inline comments.Mon, Apr 16, 12:45 PM
lib/Target/X86/X86ISelLowering.cpp
238

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

craig.topper added inline comments.Mon, Apr 16, 10:16 PM
test/CodeGen/X86/sitofp.ll
4

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 added inline comments.Mon, Apr 16, 10:47 PM
lib/Target/X86/X86ISelLowering.cpp
238

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 this revision to Diff 142752.Tue, Apr 17, 3:54 AM
vrybalov edited the summary of this revision. (Show Details)

Updated test.

spatel added inline comments.Tue, Apr 17, 7:35 AM
test/CodeGen/X86/sitofp.ll
4

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 updated this revision to Diff 143309.Fri, Apr 20, 6:51 AM

The patch is updated according to review comments.

This revision is now accepted and ready to land.Fri, Apr 20, 10:50 AM