This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix use SSE registers if no-x87 is selected.
ClosedPublic

Authored by niravd on Sep 26 2018, 8:03 AM.

Details

Summary

Fix use of SSE1 registers for f32 ops in no-x87 mode.

Notably, allow use of SSE instructions for f32 operations in 64-bit
mode (but not 32-bit which is disallowed by callign convention).

Also avoid translating memset/memcopy/memmove into SSE registers
without X87 for 32-bit mode.

This fixes PR38738.

Diff Detail

Event Timeline

niravd created this revision.Sep 26 2018, 8:03 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
1929–1959

This whole block should be guarded from trying to use fp registers if the target does not have x87. The current patch overfits our bizarro test case.

Can you then add test cases for other combinations:
+avx,-x87, size >32 (L1928)
+sse2,-x87 (L1936)
+sse2,-x87,memcpy > 8B, i686 (L1942)

niravd updated this revision to Diff 167179.Sep 26 2018, 12:56 PM

Update tests

niravd updated this revision to Diff 167509.Sep 28 2018, 10:20 AM
niravd retitled this revision from [X86] Avoid SSE1 registers for memory operations if no-x87. to [X86] Fix use SSE registers if no-x87 is selected..
niravd edited the summary of this revision. (Show Details)

Update to better match GCC's behavior. Not as aggressive as we can be.

niravd updated this revision to Diff 167806.Oct 1 2018, 1:10 PM
niravd edited the summary of this revision. (Show Details)

Fix typo.

niravd added inline comments.Oct 1 2018, 1:21 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
1929–1959

Looking at the history of this more, it seems that HasX87 was meant to correspond to mno-80387 which is disabling use of the x87 coprocessor. Without X87, GCC allows SSE2 (and successors) instructions, but avoids using SSE1 registers for memset. etc.

nickdesaulniers added inline comments.Oct 1 2018, 2:14 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
282

How come not also this case? Or any of the ISD::FP_TO_SINT?

552

Below (L1947) you check Subtarget.is64Bit() || Subtarget.hasX87(), so I'm curious here why the additional !useSoftFloat()?

580–591

Is it possible to share some of this code with the case starting at L591? Seems to be a fair amount of code duplication. Maybe a helper, or a single case can be checked for these?

niravd updated this revision to Diff 167962.Oct 2 2018, 9:30 AM

Address comments.

niravd marked an inline comment as done.Oct 2 2018, 9:32 AM
niravd added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
282

Good catch. This looks was a not reverted change from a failed simplification. Reverted

552

If we're using SoftFloat we don't want f32/f64 to have an associated register class so we will fall back to the integer registers.

Ideally we'd be able to still use the SSE1 registers for the memory shuffling operations, but that requires more plumbing and GCC doesn't use XMM registers in this case so I've left it off for this.

I've tested this patch and it fixes the cases from PR38738, and I'm happy with the code changes. @craig.topper , would you mind reviewing, too?

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

Testing the raw patch file, looks like this line has trailing whitespace. Did you mean to have a trailing comment, like L603 above?

craig.topper added inline comments.Oct 2 2018, 1:21 PM
llvm/test/CodeGen/X86/pr38738.ll
13

Do we need the dso_local in these tests?

niravd updated this revision to Diff 168014.Oct 2 2018, 1:39 PM

Simplify tests. Add comment.

niravd marked an inline comment as done.Oct 2 2018, 1:39 PM
nickdesaulniers accepted this revision.Oct 2 2018, 2:08 PM

Thank you for this patch, @niravd !

This revision is now accepted and ready to land.Oct 2 2018, 2:08 PM
craig.topper accepted this revision.Oct 2 2018, 2:10 PM
This revision was automatically updated to reflect the committed changes.