Add new #[target_feature = "..."] attribute. by BurntSushi · Pull Request #38079 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation27 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This commit adds a new attribute that instructs the compiler to emit
target specific code for a single function. For example, the following
function is permitted to use instructions that are part of SSE 4.2:

#[target_feature = "+sse4.2"]
fn foo() { ... }

In particular, use of this attribute does not require setting the
-C target-feature or -C target-cpu options on rustc.

This attribute does not have any protections built into it. For example,
nothing stops one from calling the above foo function on hosts without
SSE 4.2 support. Doing so may result in a SIGILL.

I've also expanded the x86 target feature whitelist.

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

Here are some questions/comments/concerns:

There is no validation of the values in target_feature. They are passed directly to LLVM at codegen time. This means errors (if any) are reported at codegen time.
The only test here is a compile-fail test that checks that target_feature is gated. I wanted to write a codegen test, but I don't quite know how for this particular change.
Should it be unsafe to call a function decorated with the target_feature attribute? (A different phrasing: should application of target_feature be limited to unsafe functions?)

I don't know whether any of the above should be fixed before merging. Certainly, they should at least be fixed/addressed before stabilization.

@@ -60,11 +62,10 @@ pub fn set_frame_pointer_elimination(ccx: &CrateContext, llfn: ValueRef) {
// FIXME: #11906: Omitting frame pointers breaks retrieving the value of a
// parameter.
if ccx.sess().must_not_eliminate_frame_pointers() {
let name = CString::new("no-frame-pointer-elim").unwrap();
let val = CString::new("true").unwrap();

Choose a reason for hiding this comment

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

I think you can still use CStr with null-terminated &str.

for attr in attrs {
if attr.check_name("cold") {
if attr.check_name("target_feature") {
let val = attr.value_str().map_or(String::new(), \|s

Choose a reason for hiding this comment

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

This shouldn't allocate. You can use let val = attr.value_str().map(|s| s.as_str()); ... val.map_or("", |s| &s[..]).split(",").

Choose a reason for hiding this comment

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

It turns out this doesn't work because s is an InternedString. I just took the hit with the extra case analysis instead.

for feat in val.split(",") {
let feat = feat.trim().to_string();
if !feat.is_empty() && !feat.contains('\0') {
target_features.push(feat);

Choose a reason for hiding this comment

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

You can move the .to_string() on this line, or push to a String instead of allocating for each one.

@@ -88,4 +97,10 @@ pub fn from_fn_attrs(ccx: &CrateContext, attrs: &[ast::Attribute], llfn: ValueRe
unwind(llfn, true);
}
}
if !target_features.is_empty() {
let name = CString::new("target-features").unwrap();

Choose a reason for hiding this comment

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

Same here, I'm pretty sure you could avoid the allocation.

Choose a reason for hiding this comment

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

Yeah, I just didn't really care about the allocation. I'll fix them. :-)

The x86 whitelist has ramifications elsewhere for the stable name of these features. The intention is that we ourselves maintain a mapping of features to the actual LLVM names, so that way if LLVM changes a name we can just update our implementation but the name we present to Rust is always stable.

In that sense could we perhaps be more conservative with the update to the x86 whitelist? Perhaps only the features needed for SIMD progress for now?

(We will definitely want to add more from that list at time moves on. AVX512 has a particularly large presence, but I think we should punt on AVX512 for this initial push. It it absolutely gigantic.)

This commit adds a new attribute that instructs the compiler to emit target specific code for a single function. For example, the following function is permitted to use instructions that are part of SSE 4.2:

#[target_feature = "+sse4.2"]
fn foo() { ... }

In particular, use of this attribute does not require setting the -C target-feature or -C target-cpu options on rustc.

This attribute does not have any protections built into it. For example, nothing stops one from calling the above foo function on hosts without SSE 4.2 support. Doing so may result in a SIGILL.

This commit also expands the target feature whitelist to include lzcnt, popcnt and sse4a. Namely, lzcnt and popcnt have their own CPUID bits, but were introduced with SSE4.

I think I'd agree that we should require target_feature functions to be unsafe.

EDIT: Hmm, maybe not - I don't think you'd see anything worse than a crash, and a crash isn't memory unsafe, right?

To be more precise, I think the failure mode is that your program attempts to execute an instruction that the CPU doesn't recognize. Is this behavior actually defined to always crash your program?

@BurntSushi If the kernel doesn't emulate it then you get a SIGILL on most UNIX systems (I would hope).
Paging @retep998 for windows behavior (I'd assume it's a SEH exception or something similar).

If you attempt to execute an illegal instruction on Windows, a STATUS_ILLEGAL_INSTRUCTION exception is fired.

Note that "target-features" function attribute is undocumented. So while LLVM is unstable, this attribute is extra unstable, even unstable by LLVM standard.

At least in 2015, a bug about "target-features" function attribute was closed as "LATER". See LLVM bug 22081. Hopefully this changed, but I haven't seen any announcement. Did anyone?

@sanxiyn If it's good enough for Clang and LDC, is it good enough for Rust? (I note that gcc seems to support a similar---possibly identical---feature.)

As a side note, our current trajectory for SIMD stabilization hinges pretty critically on #[target_feature]. Without it, we're pretty much back to the drawing board. (One possibility would be to keep the attribute unstable and only use it in std, which would get us somewhere, but that seems unfortunate.)

I don't know what the proper channels are to inquire about something like this.

I am simply pointing out two things. One, the attribute is unstable in LLVM. Of course, that doesn't mean it can't be stable in Rust. In principle, Rust is already using LLVM C++ API, which is also unstable.

Two, #[target_feature] and -C target-feature are not equivalent. They should be, but for various reasons they aren't at the moment. And as far as I know LLVM upstream is not very interested in fixing cases when they aren't equivalent.

r+ from me on this. I'm fine on punting on requiring unsafe. This is an unstable feature for now and we'll spec it out more before stabilizing. In the time being it allows playing around with SIMD and exploring the possible design space there.

I'm also fine using something unstable in LLVM, especially in an unstable API. Presumably the clang feature of __attribute__ is "stable", so there will always be some way of doing this. If that changes over time in LLVM revisions then it's no different than anything else we use from LLVM, really.

@eddyb do all the trans pieces here look good to you?

Yeah, it looks good to me.

📌 Commit 80ef1db has been approved by alexcrichton

bors added a commit that referenced this pull request

Add new #[target_feature = "..."] attribute.

This commit adds a new attribute that instructs the compiler to emit target specific code for a single function. For example, the following function is permitted to use instructions that are part of SSE 4.2:

#[target_feature = "+sse4.2"]
fn foo() { ... }

In particular, use of this attribute does not require setting the -C target-feature or -C target-cpu options on rustc.

This attribute does not have any protections built into it. For example, nothing stops one from calling the above foo function on hosts without SSE 4.2 support. Doing so may result in a SIGILL.

I've also expanded the x86 target feature whitelist.

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing 2cdbd5e to master...

bors mentioned this pull request

2 tasks

bors mentioned this pull request