This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add FeatureCMOV explicitly to all CPUs that support it. Remove FeatureCMOV implication from Feature64Bit and FeatureSSE1
ClosedPublic

Authored by craig.topper on Aug 24 2018, 11:50 AM.

Details

Summary

Previously most CPUs inherited cmov support through Feature64Bit(or FeatureCMPXCHG16HB implying Feature64Bit) or FeatureSSE1.

This has the surprising side effect that -mattr=-cmov causes an assert to fire in 64-bit mode because it clears the Feature64Bit. Or in 32-bit mode, -mattr=-cmov disables any sse/avx features which seems surprising.

This patch removes the implication and instead updates hasCMOV in X86Subtarget to check SSE1 or is64Bit in addition to the regular cmov flag. This should keep most things working the way they did before. I don't believe there is a way to specific "-cmov" directly from clang so this should only effect our lower level tools.

This does stop -mattr=cx16(cmpxchg16b) from implying cmov is enabled via the 64bit flag as you can see from one of the changed tests. But that was a 32-bit test so I don't know why it enabled cx16 anyway.

For the other test I had to add -sse to override the new sse check in hasCMOV.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 24 2018, 11:50 AM

Does anything equivalent need to be done in clang?

lib/Target/X86/X86.td
912

Is this an independent fix?

Clang doesn't seem to know about the cmov feature at all so I don't think there's anything needed there.

lib/Target/X86/X86.td
912

Looks like I may have gotten carried away with adding CMOV. Thanks.

Remove the change to "athlon". Though I'm not sure its incorrect and might be an existing bug.

Remove the change to "athlon". Though I'm not sure its incorrect and might be an existing bug.

GCC thinks that athlon/athlon-tbird support CMOV

RKSimon accepted this revision.Aug 26 2018, 5:46 AM

LGTM along with D51264

This revision is now accepted and ready to land.Aug 26 2018, 5:46 AM
This revision was automatically updated to reflect the committed changes.