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

Whitelist v7 feature for ARM and AARCH64. #47826

Merged
merged 1 commit into from
Jan 30, 2018
Merged

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jan 28, 2018

Needed for v7 features in coresimd.

See https://github.com/rust-lang-nursery/stdsimd/blob/b2f7be24d5043a88427f9a5258ca9a51ede6d029/coresimd/src/arm/v7.rs#L40 which used to work but doesn't anymore.

r? alexcrichton

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 28, 2018

r? @alexcrichton

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 28, 2018
@alexcrichton
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jan 28, 2018

📌 Commit b32dbbc has been approved by alexcrichton

kennytm added a commit to kennytm/rust that referenced this pull request Jan 29, 2018
Whitelist v7 feature for ARM and AARCH64.

Needed for `v7` features in `coresimd`.

See https://github.com/rust-lang-nursery/stdsimd/blob/b2f7be24d5043a88427f9a5258ca9a51ede6d029/coresimd/src/arm/v7.rs#L40 which used to work but doesn't anymore.

r? alexcrichton
kennytm added a commit to kennytm/rust that referenced this pull request Jan 30, 2018
Whitelist v7 feature for ARM and AARCH64.

Needed for `v7` features in `coresimd`.

See https://github.com/rust-lang-nursery/stdsimd/blob/b2f7be24d5043a88427f9a5258ca9a51ede6d029/coresimd/src/arm/v7.rs#L40 which used to work but doesn't anymore.

r? alexcrichton
bors added a commit that referenced this pull request Jan 30, 2018
Rollup of 12 pull requests

- Successful merges: #47515, #47603, #47718, #47732, #47760, #47780, #47822, #47826, #47836, #47839, #47853, #47855
- Failed merges:
@bors bors merged commit b32dbbc into rust-lang:master Jan 30, 2018
@Amanieu
Copy link
Member

Amanieu commented Feb 26, 2018

Why was the v7 feature added to AArch64? There is no v7 LLVM feature for AArch64 since it is assumed to always be available. I think this should be removed from the AArch64 list.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 27, 2018

What should:

#[target_feature(enabled = "v7")] unsafe fn foo() {}

do on AArch64? If the v7 feature is not whitelisted, at least with the current system, this should fail to compile. EDIT: Those targeting both AArch64 and ARM would need to write:

#[cfg_attr(target_arch = "arm", target_feature(enabled = "v7")]
unsafe fn foo() {}

@Amanieu
Copy link
Member

Amanieu commented Feb 27, 2018

It should fail to compile. You should use #[cfg_attr(target_arch = "arm", target_feature(enabled = "v7")] instead: enable v7 on ARM, no-op on AArch64.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 27, 2018

I would be fine with that, but the original reason this was white-listed was for code that works on ARM to work on AArch64 "as is", without modifications.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 27, 2018

If this is an issue, probably this should be made consistent for x86 as well. There are many features in x86_64 that are enabled by default on top of x86 (sse, sse2, ...). @alexcrichton that would mean that writing #[target_feature(enable = "sse")] on x86_64 would need to become an error.

@Amanieu
Copy link
Member

Amanieu commented Feb 27, 2018

I don't think this really makes sense for ARM. x86 and x86_64 are practically identical instruction sets and share the same features. However ARM and AArch64 are effectively completely different instruction sets, and have very different feature lists.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 27, 2018

Isn't AArch64 a super-set of ARM ? All the ARM intrinsics that we have currently implemented are available on AArch64 as well (same name, same llvm intrinsic name, same assembly instruction name, etc.).

@BubbaSheen
Copy link

So exactly what is the issue with this coding?? Asking strictly out of curiosity.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 27, 2018

IIUC the problem is that v7 is not a feature that can be enabled / disabled on AArch64, so it does not make much sense to have it as such.

@Amanieu
Copy link
Member

Amanieu commented Feb 27, 2018

Isn't AArch64 a super-set of ARM ? All the ARM intrinsics that we have currently implemented are available on AArch64 as well.

This is only actually true for the NEON intrinsics. Many of the remaining intrinsics are only available in the 32-bit instruction set, e.g. DSP (__smlabb), integer saturation (__ssat), 32-bit SIMD (__sadd8, int8x4_t)...

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 27, 2018

This is only actually true for the NEON intrinsics.

The bit manipulation intrinsics (rbit and friends) are not part of neon and IIRC and are available on both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants