Page MenuHomePhabricator

[X86] Don't use fsin/fcos/fsincos instructions ever
ClosedPublic

Authored by craig.topper on Aug 4 2017, 5:10 PM.

Details

Summary

Previously we would use these instructions if sse was disabled and fastmath was enabled.

As mentioned in D28335, this is a bad idea.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Aug 4 2017, 5:10 PM
DavidKreitzer edited edge metadata.Aug 9 2017, 11:43 AM

Just a minor question on the test changes. Otherwise, this LGTM.

test/CodeGen/X86/sincos-opt.ll
124 ↗(On Diff #109839)

Was moving the check for 'ret' here intentional?

craig.topper added inline comments.Aug 9 2017, 11:45 AM
test/CodeGen/X86/sincos-opt.ll
124 ↗(On Diff #109839)

I think all I did was copy the GNU_SINCOS check lines from above and changed them to say GNU_SINCOS_FASTMATH. I hadn't noticed the difference in the ret.

craig.topper added inline comments.Aug 11 2017, 10:41 AM
test/CodeGen/X86/sincos-opt.ll
124 ↗(On Diff #109839)

Do you want me to add it back or is it ok like this?

DavidKreitzer accepted this revision.Aug 11 2017, 11:14 AM
DavidKreitzer added a subscriber: zvi.

Adding Zvi as an FYI, but I think this is fine to land.

test/CodeGen/X86/sincos-opt.ll
124 ↗(On Diff #109839)

As far as I'm concerned it's okay either way.

This revision is now accepted and ready to land.Aug 11 2017, 11:14 AM
This revision was automatically updated to reflect the committed changes.