This is an archive of the discontinued LLVM Phabricator instance.

[X86] Improved lowering for saturating float to int.
ClosedPublic

Authored by ebevhan on Aug 17 2020, 8:00 AM.

Details

Summary

Adapted from D54696 by @nikic.

This patch improves lowering of saturating float to
int conversions, FP_TO_[SU]INT_SAT, for X86.

Diff Detail

Event Timeline

ebevhan created this revision.Aug 17 2020, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 8:00 AM
ebevhan requested review of this revision.Aug 17 2020, 8:00 AM
craig.topper added inline comments.Aug 17 2020, 11:49 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
281

SSE1 only supports f32 in SSE registers. f64 requires SSE2

21068

Drop curly braces

ebevhan updated this revision to Diff 286218.Aug 18 2020, 1:33 AM

Addressed comments.

ebevhan marked an inline comment as done.Aug 18 2020, 1:35 AM
ebevhan added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
281

I think this should deal with it. It checks the appropriate support in the lowering function as well.

craig.topper added inline comments.Aug 18 2020, 1:37 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
281

hasSSE2() implies hasSSE1() so you don't need to check both.

If you want this to work for f32 SSE1 only targets. You'll need to explicitly check hasSSE2 in the Lower function. The default CPUs on Linux, Windows, and Mac all have SSE2 enabled. So I don't know if you want to support SSE1 only or not. Our test coverage for SSE1 only isn't great.

craig.topper added inline comments.Aug 18 2020, 1:40 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
281

Sorry I wrote that before I saw your comment and I didn't check the Lowering function.

21068

You can use isScalarFPTypeInSSEReg helper function to check the type and SSE level

ebevhan updated this revision to Diff 286249.Aug 18 2020, 4:39 AM

Addressed comments.

ebevhan added inline comments.Aug 18 2020, 4:41 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
281

Hm, okay. The tests for this also only use sse2, so perhaps that's safer.

21068

That's better, thanks.

This revision is now accepted and ready to land.Aug 18 2020, 11:57 AM
ebevhan updated this revision to Diff 290264.Sep 7 2020, 5:45 AM

Rebased.

uabelho added a subscriber: uabelho.Oct 9 2020, 1:46 AM
This revision was landed with ongoing or failed builds.Jan 12 2021, 6:45 AM
This revision was automatically updated to reflect the committed changes.
bjope added a comment.Jan 12 2021, 6:46 AM

I landed this on behalf of @ebevhan