Page MenuHomePhabricator

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

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



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

Event Timeline

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

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.Apr 16 2018, 7:42 AM

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

vrybalov added inline comments.Apr 16 2018, 7:49 AM

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

efriedma added inline comments.Apr 16 2018, 12:45 PM

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

craig.topper added inline comments.Apr 16 2018, 10:16 PM

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.Apr 16 2018, 10:47 PM

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

Updated test.

spatel added inline comments.Apr 17 2018, 7:35 AM

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.Apr 20 2018, 6:51 AM

The patch is updated according to review comments.

This revision is now accepted and ready to land.Apr 20 2018, 10:50 AM
This revision was automatically updated to reflect the committed changes.
lebedev.ri added inline comments.
17 ↗(On Diff #143722)

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

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.

17 ↗(On Diff #143722)

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