Enable CMPXCHG16B, SSE3, SAHF/LAHF and 128-bit Atomics (in nightly) in Windows x64

[CKingX]

As Rust plans to set Windows 10 as the minimum supported OS for target x86_64-pc-windows-msvc, I have added the cmpxchg16b and sse3 feature. Windows 10 requires CMPXCHG16B, LAHF/SAHF, and PrefetchW as stated in the requirements here. Furthermore, CPUs that meet these requirements also have SSE3 (see)

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fmease (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[CKingX]

Strange. rustc --print=target-features lists cmpxchg16b

[ChrisDenton]

+sahf too?

[CKingX]

That I am not sure. x86-64-v2 onwards don't use sahf though it is listed in the features. CMPXCHG16B and SSE3 are listed. Does that mean Rustc doesn't yet make use of these instructions?

[CryZe]

Not sure what you are seeing but x86-64-v2 totally does specify SAHF:
https://github.com/llvm/llvm-project/blob/a9700904765590ca2fbf08c0cc36d0da1107d3a7/llvm/lib/Target/X86/X86.td#L808-L809

[CKingX]

I used this

rustc --print=cfg -C target-cpu=x86-64-v2
debug_assertions
panic="unwind"
target_arch="x86_64"
target_endian="little"
target_env="msvc"
target_family="windows"
target_feature="cmpxchg16b"
target_feature="fxsr"
target_feature="popcnt"
target_feature="sse"
target_feature="sse2"
target_feature="sse3"
target_feature="sse4.1"
target_feature="sse4.2"
target_feature="ssse3"
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="64"
target_has_atomic="8"
target_has_atomic="ptr"
target_os="windows"
target_pointer_width="64"
target_vendor="pc"
windows

Did some more digging and turns out SAHF seems to be for kernel rather than userspace code. Hence it is not listed in the rustc command I ran above.

[CKingX]

It also seems to be tied to virtualization and for kernel space.

[ChrisDenton]

Ok, I've been researching this a bit more. On balance I do think we should add +sahf. While I don't think it's that important, it was seen fit to be included in x86-64-v2 feature level and I think we should align ourselves with that (although obviously dropping the SSE level back down to 3).

[CKingX]

done!

[ChrisDenton]

Hm, using +cx16 appears to work but for some reason cmpxchg16b doesn't.

EDIT: Ah, to_llvm_features has a list of features that need translating from their rust names to their llvm names. This isn't applied to target specs so you have to use the llvm name manually.

[CKingX]

Thank you! Fixed it

[CKingX]

Also as CMPXCHG16B allows 128-bit/16-byte atomics, I am creating a new commit to update max atomic width as well. This is also already done for x86_64_apple_darwin target where it states:
base.max_atomic_width = Some(128); // penryn+ supports cmpxchg16b

[ChrisDenton]

Hm, doesn't that also require avx for full atomics?

[CKingX]

x86-64 darwin (macOS) target does this without AVX support as the minimum requirement is Penryn (Core 2) which doesn't support AVX or AVX2.

[ChrisDenton]

Fair enough. Maybe I was thinking of something else.

[mati865]

Shouldn't this be set for all other x86 Windows targets as well?

[CKingX]

As for 32-bit Windows 10, it only requires SSE2 and that is already the requirement in Rustc.

Now for 64-bit MinGW ABI, I could update it if that also requires Windows 10 and up. I looked at Rust platforms and I didn't see win7 version of MinGW ABI so I am unsure. However, if the -gnu targets also dropped support, I can do a new commit

[ChrisDenton]

All x86_64-pc-windows targets with std support are raising their baseline to Windows 10. Also the uwp targets because they inherently don't support older Windows versions. The only exceptional one is the win7 target.

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

• 655dccd

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

• 74f246c

[bors]

⌛ Testing commit 376c7b9 with merge d6a09a7...

[bors]

💔 Test failed - checks-actions

[CKingX]

Ok so the test is checking if SSE4.2 enables CRC32 on x86-64 (base). As we passed in extra features, it fails.

I am going to look into how other targets handle this when they add feature flags

[CKingX]

I did some digging and it is not all good news. x86_64h-apple-darwin handles this by using core-avx2 and removing some features using the "-feature" syntax. Now, the features list is evaluated the feature left to right so "-feature,+feature" should still lead to it correcting when you add the feature. The disadvantage of this approach is that it doesn't work if you change the target cpu (like choosing native) where those features don't get added back. Other platforms like x86-64 Fuschia and Android add features just fine

That leaves updating the test to exclude windows target, but I am unsure on if the test should be changed. At the very least, we should add another test for windows x64.

[CKingX]

The other idea is setting the target cpu as nocona but that might mess up optimizations as a whole because nocona is based off of pentium 4 (far off from modern CPUs as compared to core to haswell)

[erikdesjardins]

From the test failure it looks like that line changes to

attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable "target-cpu"="x86-64" "target-features"="+cx16,+sse3,+sahf,+sse4.2,+crc32" }

So it's fine to just change the test expectation to allow additional target features before sse4.2/crc32, like

// CHECK: attributes #0 {{.*"target-features"=".*\+sse4.2,\+crc32"}}

since that upholds the spirit of the test (that adding sse4.2 also adds crc32)

[CKingX]

Thanks erikdesjardins! Updating the test was a lot simpler than I expected.

[CKingX]

Just as a heads up the updated test just needs to be reviewed

[ChrisDenton]

lgtm, thanks!

@bors r=petrochenkov,ChrisDenton

[bors]

📌 Commit 2d25c3b has been approved by petrochenkov,ChrisDenton

It is now in the queue for this repository.

[CKingX]

Thank you Chris, erikdesjardins, CryZe and petrochenkov!