This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add strict fp support for operations of X87 instructions
ClosedPublic

Authored by LiuChen3 on Oct 10 2019, 8:03 PM.

Details

Summary

This is the following patch of D68854.

This patch adds basic operations of X87 instructions, including +, -, *, / , fp extensions and fp truncations.

Diff Detail

Event Timeline

LiuChen3 created this revision.Oct 10 2019, 8:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 8:03 PM
craig.topper added inline comments.Oct 10 2019, 8:28 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
610

Doesn't this stop working if sse1 is enabled? By we still need x87 for f64/f80.

llvm/lib/Target/X86/X86InstrFPStack.td
373

SIN/COS are not mentioned in the X86ISelLowering code you added.

llvm/test/CodeGen/X86/x87-fp-strict-sub.ll
85

Is all this metadata needed?

LiuChen3 marked an inline comment as done.Oct 10 2019, 10:19 PM
LiuChen3 added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
610

Yes . This can only be enabled when disable sse. I'll find which will still be needed when SSE enabled. I think most of them are related to f80.

llvm/lib/Target/X86/X86InstrFPStack.td
373

X86 uses library to calculate SIN/COS. X87 set SIN/COS as Expand. So I leave strict_fsin/strict_fcos as "expand" as default.

llvm/test/CodeGen/X86/x87-fp-strict-sub.ll
85

It's not necessay. I'll delete them.

craig.topper added inline comments.Oct 10 2019, 10:32 PM
llvm/lib/Target/X86/X86InstrFPStack.td
373

Ok then don't change these lines. We shouldn't have entries in the isel table that we don't need.

LiuChen3 marked an inline comment as done and an inline comment as not done.Oct 10 2019, 10:34 PM
LiuChen3 added inline comments.
llvm/lib/Target/X86/X86InstrFPStack.td
373

ok

LiuChen3 updated this revision to Diff 224553.Oct 11 2019, 1:53 AM
LiuChen3 updated this revision to Diff 228805.Nov 11 2019, 8:48 PM

Rebase and change the name of tests in fp-strict-scalar.ll . Add a test file which only process 80-bit long double type, because tests for 80-bit FP is complex to put into fp-strict-scalar.ll without changing the '–check-prefix'.

craig.topper added inline comments.Nov 11 2019, 9:08 PM
llvm/test/CodeGen/X86/fp-strict-scalar.ll
23 ↗(On Diff #228805)

How about fadd_f64 and fadd_f32 instead of fadd1/fadd2?

llvm/test/CodeGen/X86/fp80-strict-scalar.ll
2 ↗(On Diff #228805)

I don't think we need to test all these combinations. But if you want to keep them, I suggest adding a CHECK-X86 and CHECK-X64 prefix so that we get more commonality.

22 ↗(On Diff #228805)

fadd_f80

166 ↗(On Diff #228805)

fpext_f32_to_f80

LiuChen3 marked an inline comment as done.Nov 11 2019, 9:19 PM
LiuChen3 added inline comments.
llvm/test/CodeGen/X86/fp80-strict-scalar.ll
2 ↗(On Diff #228805)

How about just keep the "; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=-sse -O3 | FileCheck %s --check-prefixes=CHECK,X87". I think fp80 mostly uses X87 instructions.

LiuChen3 updated this revision to Diff 228990.Nov 12 2019, 6:10 PM

Modify the name of tests and delete some RUN line in fp80-strict-scalar.ll.

LiuChen3 updated this revision to Diff 228993.Nov 12 2019, 6:20 PM

Modify the name of tests

I think we need to re-evaluate our testing strategy here. These tests pass on trunk without any of the other changes.

I was going to suggest adding -stop-after=finalize-isel so we could check the fpexcept bit, but the mutating code preserves the fpexcept bit from the strict fp node we mutated. So I'm not sure how we can tell that this patch works correctly.

This comment was removed by pengfei.

How about we add a command line option to SelectionDAGISel to disable the mutation code and pass that option on the tests?

OK, I 'll make a try.

How about we add a command line option to SelectionDAGISel to disable the mutation code and pass that option on the tests?

I think it's reasonable.

The new tests and test renames can be done as an NFC commit - then please rebase this patch

The new tests and test renames can be done as an NFC commit - then please rebase this patch

Sorry, I am not clear of what you mean. Do you mean I should make a new patch to add these tests? The tests in my current patch is not just renamed, but also doing functional test. I think it's better to keep them together.
And I think make a new patch to rename the tests is reasonable.

The new tests and test renames can be done as an NFC commit - then please rebase this patch

Sorry, I am not clear of what you mean. Do you mean I should make a new patch to add these tests? The tests in my current patch is not just renamed, but also doing functional test. I think it's better to keep them together.
And I think make a new patch to rename the tests is reasonable.

I've updated/added the tests in rG5aaca2355ec2 (matching Craig's changes in rG0cc12b8a8310) - the changes were an NFC cleanup and don't require a new patch. Please can you now rebase?

The new tests and test renames can be done as an NFC commit - then please rebase this patch

Sorry, I am not clear of what you mean. Do you mean I should make a new patch to add these tests? The tests in my current patch is not just renamed, but also doing functional test. I think it's better to keep them together.
And I think make a new patch to rename the tests is reasonable.

I've updated/added the tests in rG5aaca2355ec2 (matching Craig's changes in rG0cc12b8a8310) - the changes were an NFC cleanup and don't require a new patch. Please can you now rebase?

I have rebase. Thanks.

craig.topper added inline comments.Nov 25 2019, 11:53 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
5227 ↗(On Diff #230838)

This isn't the right check for the fp80 operations. They're independent of sse.

5228 ↗(On Diff #230838)

Need an LLVM_FALLTHROUGH marker here

llvm/test/CodeGen/X86/fp80-strict-scalar.ll
3 ↗(On Diff #230838)

Why are we disabling sse on x86_64?

LiuChen3 marked an inline comment as done.Nov 25 2019, 4:30 PM
LiuChen3 added inline comments.
llvm/test/CodeGen/X86/fp80-strict-scalar.ll
3 ↗(On Diff #230838)

Make sure we are using X87 instructions. This option is actually useless when using fp80, I'll delete it.

LiuChen3 updated this revision to Diff 230999.Nov 25 2019, 8:34 PM

Rebase.
STRICT_FP_ROUND has been set as custom so I remove setting it as legal. When entering the custom processing function, if the operand type is legal,the function behaves the same as setOperationAction(ISD:STRICT_FP_ROUND, TYPE, legal)
Modify the test case. Since we do not disable sse, we can pass float-pointer value by register instead of passing by pointer.

Rebase.
STRICT_FP_ROUND has been set as custom so I remove setting it as legal. When entering the custom processing function, if the operand type is legal,the function behaves the same as setOperationAction(ISD:STRICT_FP_ROUND, TYPE, legal)
Modify the test case. Since we do not disable sse, we can pass float-pointer value by register instead of passing by pointer.

STRICT_FP_ROUND should only be Custom on a 64-bit target. This probably only works because LegalizeDAG bailed out of ExpandNode due to strictfp mutation being disabled. And there is no libcall support for STRICT_FP_ROUND so LegalizeDAG gave up and just left the node alone.

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
5235 ↗(On Diff #230999)

I think this code should also call TLI.isStrictFPEnabled() before doing the mutate.

5227 ↗(On Diff #230838)

For FP_ROUND isn't the input type f80 not the result type?

LiuChen3 marked an inline comment as done.Nov 25 2019, 8:58 PM
LiuChen3 added inline comments.
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
5228 ↗(On Diff #230838)

I think there is still some problems here, I'll work on it

Please pre-commit the test modifications with the old codegen and rebase this patch.

Please pre-commit the test modifications with the old codegen and rebase this patch.

OK. Thanks for your help.

LiuChen3 updated this revision to Diff 231007.Nov 25 2019, 11:16 PM

Pengfei has helped me commit the test cases.
I rebase and add the option "-disable-strictnode-mutation" to the test case.

LiuChen3 marked an inline comment as done.Nov 25 2019, 11:31 PM
LiuChen3 added inline comments.
llvm/test/CodeGen/X86/fp-strict-scalar.ll
18 ↗(On Diff #231007)

Hi, Craig. Is the sequence of "fpext.f64.f32" important? I thought it should be fpext.sourcetype.destinationtype, but fpext.f64.f32 can output right code, too.
Did this Intrinsic only use pass value type fpext.f64.f32(float, metadata) and return value type double to determine the source type and return type?

craig.topper added inline comments.Nov 25 2019, 11:51 PM
llvm/test/CodeGen/X86/fp-strict-scalar.ll
18 ↗(On Diff #231007)

Its supposed to be fpext.destinationtype.sourcetype. The types are determined by the order of the llvm_anyint_ty/llvm_anyfloat_ty listed in the Intrinsics.td file with output types before input types.

But I think the parser doesn't rely on the types in the name and just use the types mentioned. I bet if you feed the test into 'opt' with no other arguments when it gets printed back out the name will have been fixed.

LiuChen3 marked an inline comment as done.Nov 26 2019, 1:00 AM
LiuChen3 added inline comments.
llvm/test/CodeGen/X86/fp-strict-scalar.ll
18 ↗(On Diff #231007)

Yes, you are right. opt will fix the problems.
Then llvm.experimental.constrained.fptrunc has wrong sequence. Should I update the testcase in this patch or pre-commit the testcase and rebase this patch?

This revision is now accepted and ready to land.Nov 26 2019, 10:48 AM
This revision was automatically updated to reflect the committed changes.