Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove inline assembly from hint::spin_loop #59239

Merged
merged 1 commit into from
Mar 24, 2019

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Mar 16, 2019

This PR removes the inline assembly which was not required since these
instructions are available in core::arch, and extends support of
the spin_loop hint to arm targets with the v6 feature which also
support the yield instruction.

@rust-highfive
Copy link
Collaborator

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2019
@gnzlbg gnzlbg force-pushed the fix_spin_loop branch 4 times, most recently from 292ffad to e07d163 Compare March 16, 2019 17:24
@Fanael
Copy link

Fanael commented Mar 17, 2019

The claim that pause is undefined is incorrect: pause is defined in a way that makes it a harmless NOP on all earlier CPUs, and is even explicitly defined as such in Intel manuals:

This instruction was introduced in the Pentium 4 processors, but is backward compatible with all IA-32 processors. In earlier IA-32 processors, the PAUSE instruction operates like a NOP instruction.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 17, 2019

Indeed. I suppose that this means that an assembler, like the LLVM assembler, cannot deduce from seeing pause that SSE2 is enabled, is that correct?

When trying to compile libcore with non-LLVM backends (like Cranelift), inline assembly is problematic (e.g. Cranelift does not support LLVM inline assembly). The _mm_pause intrinsic is better, but calling that intrinsic is UB on hardware that does not support SSE2.

@Fanael is there an advantage of emitting pause over not emitting anything on targets without SSE2 ? If the answer is no, then we can leave it like this. If the answer is yes, we might need to add a nop intrinsic to core::arch and call it on such targets instead.

@Fanael
Copy link

Fanael commented Mar 17, 2019

The _mm_pause intrinsic is better, but calling that intrinsic is UB on hardware that does not support SSE2.

It really is not. The intrinsic is defined in Intel manuals in terms of the instruction itself (the intrinsics in LLVM and everywhere else just follow Intel definitions), and LLVM itself doesn't require SSE2 for pause: llvm-mirror/llvm@a8f11c1

@Fanael is there an advantage of emitting pause over not emitting anything on targets without SSE2 ? If the answer is no, then we can leave it like this.

Not that I know, there is no advantage on any of Intel's old microarchitectures, they just interpret it as a nop and have no special way of handling spin loops. It might be different on AMD's K7 or VIA Nehemiah, but I don't have that hardware to test, and I doubt it makes any difference anyway.

@shepmaster
Copy link
Member

r? @alexcrichton

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 17, 2019

It really is not.

It is in Rust, due to what appears to be a bug in the Intel C Intrinsics spec that we use for our definitions. I've opened rust-lang/stdarch#705 to track this.

Not that I know, there is no advantage on any of Intel's old microarchitectures,

Then I think I would prefer to not emit anything on those, at least until the bug in core::arch is fixed.

@gnzlbg gnzlbg changed the title Fix undefined behavior in hint::spin_loop for x86 targets without SSE2 Remove inline assembly from hint::spin_loop Mar 17, 2019
@nagisa
Copy link
Member

nagisa commented Mar 18, 2019

The implementation LGTM.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 18, 2019

📌 Commit e07d163 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 19, 2019
Remove inline assembly from hint::spin_loop

This PR removes the inline assembly which was not required since these
instructions are available in core::arch, and extends support of
the spin_loop hint to arm targets with the v6 feature which also
support the yield instruction.
@Centril
Copy link
Contributor

Centril commented Mar 20, 2019

Failed in #59300 (comment), @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 20, 2019
@mati865
Copy link
Contributor

mati865 commented Mar 21, 2019

Submodules messed up?

The pause instruction requires SSE2 but was being unconditionally used
on targets without it, resulting in undefined behavior.

This PR fixes that by only using the pause intrinsic if SSE2 is available.

It also removes the inline assembly which was not required since these
instructions are available in core::arch, and extends support of
the spin_loop hint to arm targets with the v6 feature which also
support the yield instruction.

Closes rust-lang#59237 .
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 21, 2019

Submodules messed up?

Indeed, thanks!

@alexcrichton
Copy link
Member

@bors: r=nagisa

@bors
Copy link
Contributor

bors commented Mar 21, 2019

📌 Commit 830c98d has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 21, 2019
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Mar 22, 2019
Remove inline assembly from hint::spin_loop

This PR removes the inline assembly which was not required since these
instructions are available in core::arch, and extends support of
the spin_loop hint to arm targets with the v6 feature which also
support the yield instruction.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Mar 23, 2019
Remove inline assembly from hint::spin_loop

This PR removes the inline assembly which was not required since these
instructions are available in core::arch, and extends support of
the spin_loop hint to arm targets with the v6 feature which also
support the yield instruction.
kennytm added a commit to kennytm/rust that referenced this pull request Mar 24, 2019
Remove inline assembly from hint::spin_loop

This PR removes the inline assembly which was not required since these
instructions are available in core::arch, and extends support of
the spin_loop hint to arm targets with the v6 feature which also
support the yield instruction.
bors added a commit that referenced this pull request Mar 24, 2019
Rollup of 7 pull requests

Successful merges:

 - #59213 (Track changes to robots.txt)
 - #59239 (Remove inline assembly from hint::spin_loop)
 - #59251 (Use a valid name for graphviz graphs)
 - #59296 (Do not encode gensymed imports in metadata)
 - #59328 (Implement specialized nth_back() for Box and Windows.)
 - #59355 (Fix ICE with const generic param in struct)
 - #59377 (Correct minimum system LLVM version in tests)
@bors bors merged commit 830c98d into rust-lang:master Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants