This is an archive of the discontinued LLVM Phabricator instance.

[X86] Enable STRICT_FP_TO_SINT/UINT on X86 backend.
ClosedPublic

Authored by LiuChen3 on Dec 17 2019, 1:32 AM.

Details

Diff Detail

Event Timeline

LiuChen3 created this revision.Dec 17 2019, 1:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2019, 1:32 AM
craig.topper added inline comments.Dec 17 2019, 11:27 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
847

Use Res.getValue(0) and Res.getValue(1) to shorten the code.

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

Put this with its non-STRICT line

1364–1365

Don't we need to promote STRICT_FP_TO_SINT/UINT here?

1378

Blank line here

1449–1450

Need to prmote the strict notes too

1451

Put these next to their equivalent non-STRICT lines.

19756–19757

Since we removed the fallthrough out of the if, just move this down to where its used.

19761

Make this a local variable to this block so you can move the earlier declaration below this if

19774

If the IsStrict is taken, then this does an extract_subvector from a merge values and then deosn't return the chain. Are we missing test coverage? I think that should have triggered an assert.

19808

Just move this into the earlier IsStrict if. No need for back to back ifs with the same condition

28666

Please add a FIXME here, setting this to true unconditionally will need to be fixed when we stop losing flags.

LiuChen3 marked 2 inline comments as done.Dec 17 2019, 6:53 PM
LiuChen3 added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
19774

I think I made a mistake here.

28666

I forgot to delete.

LiuChen3 updated this revision to Diff 234442.Dec 17 2019, 6:55 PM

Add promotion for TRICT_FP_TO_SINT/UINT.
Modify as comment

craig.topper added inline comments.Dec 17 2019, 7:25 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
616

Move this above the if and make the non-STRICT path just return Promoted?

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

Move these two SINT_TO_FP/UINT_TO_FP lines below the STRICT lines. Better to keep the FP->INT lines stuff together.

1465

Please keep these in the same order as their non-STRICT versions. Maybe put the STRICT line directly below its corresponding non-STRICT lines. We should make it easy to see that STRICT and non-STRICT are the same.

19771

else should be at the end of the line above.

19779–19787

Move else to the line above

19806

Drop the else since we already returned in the if body.

llvm/test/CodeGen/X86/vec-strict-fptoint-512.ll
4

Should we have avx512dq coverage for the conversion to vXi64? Same with other vector tests.

LiuChen3 updated this revision to Diff 234453.Dec 17 2019, 9:26 PM
LiuChen3 marked an inline comment as done.

Modify as comments.
Add check for avx512dq

llvm/test/CodeGen/X86/vec-strict-fptoint-512.ll
4

Ok.

LiuChen3 marked an inline comment as done.Dec 17 2019, 9:48 PM
LiuChen3 added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
1167

Forget to update the testcase. I'll update later.

LiuChen3 updated this revision to Diff 234459.Dec 17 2019, 9:51 PM

update testcase

This revision is now accepted and ready to land.Dec 17 2019, 11:11 PM
pengfei added inline comments.Dec 18 2019, 1:03 AM
llvm/test/CodeGen/X86/vec-strict-fptoint-128.ll
398

Just noticed: is this an unsafe behavior for strict FP, because the upper elements may cause exceptions?

LiuChen3 marked an inline comment as done.Dec 18 2019, 2:08 AM
LiuChen3 added inline comments.
llvm/test/CodeGen/X86/vec-strict-fptoint-128.ll
398

Maybe we can set these strict-fptoint legal only when target supports avx512vl and avx512dq?

craig.topper added inline comments.Dec 18 2019, 2:16 PM
llvm/test/CodeGen/X86/vec-strict-fptoint-128.ll
398

Can we use 0.0 to pad instead of undef?

LiuChen3 marked an inline comment as done and an inline comment as not done.Dec 18 2019, 4:44 PM
LiuChen3 added inline comments.
llvm/test/CodeGen/X86/vec-strict-fptoint-128.ll
398

I think it's reasonable. I'll work on it.

This revision was automatically updated to reflect the committed changes.