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

add rtm cpu feature intrinsics #726

Merged
merged 9 commits into from
Apr 25, 2019
Merged

Conversation

mtak-
Copy link
Contributor

@mtak- mtak- commented Apr 18, 2019

Blocked by rust-lang/rust#60060

This does not include any HLE functions (xacquire/xrelease prefixed operations).

Let me know what needs changing.

Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review, this looks good to me.

crates/core_arch/src/x86/rtm.rs Outdated Show resolved Hide resolved
crates/core_arch/src/x86/rtm.rs Outdated Show resolved Hide resolved
crates/core_arch/src/x86/rtm.rs Show resolved Hide resolved
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. Can you ping me here when the upstream PR is merged? (or did that already happen?) I'll re-run CI then and merge it when its green.

@mtak-
Copy link
Contributor Author

mtak- commented Apr 19, 2019

Thanks! It's merged, but not yet in nightly AFAICT. I'll ping you.

#[inline]
#[target_feature(enable = "rtm")]
#[cfg_attr(test, assert_instr(xtest))]
pub unsafe fn _xtest() -> u8 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just changed this to return u8 to match intel headers more directly. It is specified to always be 0 or 1.

Additionally, if/when stdsimd later adds HLE intrinsics, _xtest should be enabled if either hle or rtm is supported.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, if/when stdsimd later adds HLE intrinsics, _xtest should be enabled if either hle or rtm is supported.

I don't think this is something that can be easily supported. We need to know which features are available when generating code for this intrinsic, so it is either rtm, or hle, or both. But if its rtm or hle, then we need to generate different code depending on the answer to which one is it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the intel docs say either HLE or RTM. The instruction set reference for xtest says under "CPUID Feature Flag": HLE or RTM.

we need to generate different code depending on the answer to which one is it.

Does that mean we need a duplicate of _xtest (_hletest) in any future hle.rs module?

@gnzlbg This should be ready to run through travis now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_xtest&expand=6168 says RTM CPUID flag only.

Does that mean we need a duplicate of _xtest (_hletest) in any future hle.rs module?

That's what it would mean, but that would be very weird. It seems more likely that the link you posted is incorrect, you might want to ask Intel for clarification in their forums.

@gnzlbg gnzlbg closed this Apr 21, 2019
@gnzlbg gnzlbg reopened this Apr 21, 2019
@gnzlbg gnzlbg closed this Apr 23, 2019
@gnzlbg gnzlbg reopened this Apr 23, 2019
@mtak- mtak- force-pushed the rtm-cpu-feature branch from 7b57c7c to 9b5a550 Compare April 23, 2019 17:43
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 24, 2019

Can you rebase on top of master?

@mtak-
Copy link
Contributor Author

mtak- commented Apr 24, 2019

I have already done so. verify signatures on master is broken on CI after #733.

---- verify_all_signatures stdout ----
failed to verify `_rdtsc`
  * failed to equate: `__int64` and PrimUnsigned(64) for _rdtsc
thread 'verify_all_signatures' panicked at 'assertion failed: all_valid', crates/stdsimd-verify/tests/x86-intel.rs:186:5

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 25, 2019

Oh I thought the fix for that already had landed!

@gnzlbg gnzlbg merged commit 8b5aa3a into rust-lang:master Apr 25, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 25, 2019

Thank you @mtak- for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants