-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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 new #[target_feature = "..."] attribute. #38079
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
Here are some questions/comments/concerns:
I don't know whether any of the above should be fixed before merging. Certainly, they should at least be fixed/addressed before stabilization. |
cc @eddyb |
cc @rust-lang/compiler |
@@ -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(); |
There was a problem hiding this comment.
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|s.to_string()); |
There was a problem hiding this comment.
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(",")
.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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? |
@alexcrichton Ah whoops! I will trim it back down to what it was, but I think we should add at least |
(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.
e46652a
to
80ef1db
Compare
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). |
If you attempt to execute an illegal instruction on Windows, a |
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 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, |
r+ from me on this. I'm fine on punting on requiring I'm also fine using something unstable in LLVM, especially in an unstable API. Presumably the clang feature of @eddyb do all the trans pieces here look good to you? |
Yeah, it looks good to me. |
Ok @bors: r+ |
📌 Commit 80ef1db has been approved by |
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.
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:
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 withoutSSE 4.2 support. Doing so may result in a SIGILL.
I've also expanded the x86 target feature whitelist.