Tracking issue for target_feature 1.1 RFC #69098

nikomatsakis · 2020-02-12T15:34:23Z

LeSeulArtichaut · 2020-02-18T19:07:16Z

I've been working on this, trying to see if I would be able to implement this with my very little knowledge of the compiler. I think I can propose a PR soon.

@rustbot claim

hanna-kruppe · 2020-05-02T12:29:55Z

In #69274 (comment), @petrochenkov pointed out a complication not made clear in the RFC (nor realized by anyone during the original discussion, AFAICT): safe trait functions are explicitly excluded because we can't check all call sites, but function items implement the Fn* traits, so safe functions with target features enabled face a similar problem. That is, unless they are special-cased in some way that results in them not implementing those traits.

petrochenkov · 2020-05-02T12:35:14Z

It would probably be better to add the unsafe to #[target_feature] functions implicitly during lowering to HIR.
Implicit unsafe is already added to functions in extern blocks in the same way.

That would make the unsafety checker the only place (besides AST lowering) where they would be treated specially. Like, "yes, the function is unsafe, but we know that it's safe to call in this specific context, so the unsafe block can be omitted".

The special coercion checks would no longer be necessary in that case.

@nikomatsakis

…r=hanna-kruppe Implement RFC 2396: `#[target_feature]` 1.1 Tracking issue: rust-lang#69098 r? @nikomatsakis cc @gnzlbg @joshtriplett

@nikomatsakis

…hanna-kruppe Implement RFC 2396: `#[target_feature]` 1.1 Tracking issue: rust-lang#69098 r? @nikomatsakis cc @gnzlbg @joshtriplett

LeSeulArtichaut · 2020-05-03T13:06:09Z

Now that #69274 landed, I think we can check Implement the RFC 🙂

LeSeulArtichaut · 2020-05-03T13:06:23Z

@rustbot release-assignment

hanna-kruppe · 2020-05-08T13:00:08Z

Filed #72012 for the unsoundness discussed above (I think this is how T-lang tracking issues are supposed to be used now).

nikomatsakis · 2020-06-22T20:45:22Z

Opened #73631 to discuss the expected behavior of closures with target-feature.

calebzulawski · 2021-08-22T12:13:06Z

It looks like there haven't been any updates on this in a while--is there anything I can do to bring this closer to stabilization?

nikomatsakis · 2022-03-16T13:29:17Z

@calebzulawski I know it's almost a year later but I think this is probably ready for a stabilization report? (cc @rust-lang/lang)

LeSeulArtichaut · 2022-03-17T22:08:54Z

Tried to help by opening rust-lang/reference#1181, feedback from people who actually know what they are doing would be greatly appreciated.

While writing the PR, I noticed that #72012 is a breaking change for wasm targets, where target_features could be used on safe functions. It looks like you can still safely call target_feature functions though:

fn call_it(f: impl FnOnce()) {}

#[target_feature(enable = "simd128")]
fn foo_simd128() {}

fn main() {
    foo_simd128(); // OK
    call_it(foo_simd128); // error: the trait `FnOnce<()>` is not implemented
}

…re-11, r=estebank Stabilize `#![feature(target_feature_11)]` ## Stabilization report ### Summary Allows for safe functions to be marked with `#[target_feature]` attributes. Functions marked with `#[target_feature]` are generally considered as unsafe functions: they are unsafe to call, cannot be assigned to safe function pointers, and don't implement the `Fn*` traits. However, calling them from other `#[target_feature]` functions with a superset of features is safe. ```rust // Demonstration function #[target_feature(enable = "avx2")] fn avx2() {} fn foo() { // Calling `avx2` here is unsafe, as we must ensure // that AVX is available first. unsafe { avx2(); } } #[target_feature(enable = "avx2")] fn bar() { // Calling `avx2` here is safe. avx2(); } ``` ### Test cases Tests for this feature can be found in [`src/test/ui/rfcs/rfc-2396-target_feature-11/`](https://github.com/rust-lang/rust/tree/b67ba9ba208ac918228a18321fc3a11a99b1c62b/src/test/ui/rfcs/rfc-2396-target_feature-11/). ### Edge cases - rust-lang#73631 Closures defined inside functions marked with `#[target_feature]` inherit the target features of their parent function. They can still be assigned to safe function pointers and implement the appropriate `Fn*` traits. ```rust #[target_feature(enable = "avx2")] fn qux() { let my_closure = || avx2(); // this call to `avx2` is safe let f: fn() = my_closure; } ``` This means that in order to call a function with `#[target_feature]`, you must show that the target-feature is available while the function executes *and* for as long as whatever may escape from that function lives. ### Documentation - Reference: rust-lang/reference#1181 --- cc tracking issue rust-lang#69098 r? `@ghost`

RalfJung · 2024-11-23T15:23:19Z

Hm, actually I did discover some very surprising interaction with closures, caused by #108655 / #111836. Consider this:

#![allow(improper_ctypes_definitions)]
#![feature(target_feature_11)]

use std::mem::transmute;
#[cfg(target_arch = "x86")]
use std::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use std::arch::x86_64::*;

#[inline(never)]
#[target_feature(enable = "avx")]
unsafe extern "C" fn with_tf_c(_dummy: f32, x: __m256) {
    let val = unsafe { transmute::<_, [u32; 8]>(x) };
    dbg!(val);
}

#[target_feature(enable = "avx")]
unsafe fn with_tf() -> impl FnOnce(f32, __m256) {
    #[inline(always)]
    |dummy, x| with_tf_c(dummy, x)
}

fn main() {
    // This makes all the following target feature stuff sound.
    assert!(is_x86_feature_detected!("avx"));
    
    unsafe {
        with_tf()(0.0, transmute([1; 8]));
    }
}

Turns out that the inline(always) here disables inheriting the target features into the closure. That's really surprising since they are otherwise inherited.

The lint about vector ABI issues does trigger in the above example (I should add that as a testcase). I currently can't think of any other way this could break... but it does seem quite surprising. Should we at least warn against inline(always) on closures inside target feature functions? I think we should.

RalfJung · 2024-11-23T15:41:13Z

Looking at the logic here, just stabilizing target_feature_11 will suddenly change the behavior of all closures defined in target_feature functions: they will now start inheriting feature gates, except if they are marked inline(always). Is that truly what we want? It seems like a potentially surprising flag day...

It should be sound (apart from possible ABI issues) because for the closure to be constructed, the function containing it must have been called, which means the target feature must be available, and target features can never become un-available. Still, it should be called out loudly in the stabilization report so t-lang is aware of this when we do the FCP.

EDIT: t-lang discussed this in #73631, but it doesn't seem to discuss that we are changing the behavior of existing code the moment we are stabilizing this feature. It's probably fine, but should at least be mentioned.

calebzulawski · 2024-11-23T16:34:54Z

Hm, actually I did discover some very surprising interaction with closures, caused by #108655 / #111836. Consider this:

#![allow(improper_ctypes_definitions)]
#![feature(target_feature_11)]

use std::mem::transmute;
#[cfg(target_arch = "x86")]
use std::arch::x86::;
#[cfg(target_arch = "x86_64")]
use std::arch::x86_64::;

#[inline(never)]
#[target_feature(enable = "avx")]
unsafe extern "C" fn with_tf_c(dummy: f32, x: __m256) {
let val = unsafe { transmute::<, [u32; 8]>(x) };
dbg!(val);
}

#[target_feature(enable = "avx")]
unsafe fn with_tf() -> impl FnOnce(f32, __m256) {
#[inline(always)]
|dummy, x| with_tf_c(dummy, x)
}

fn main() {
// This makes all the following target feature stuff sound.
assert!(is_x86_feature_detected!("avx"));
unsafe {
    with_tf()(0.0, transmute([1; 8]));
}
}

Turns out that the inline(always) here disables inheriting the target features into the closure. That's really surprising since they are otherwise inherited.

The lint about vector ABI issues does trigger in the above example (I should add that as a testcase). I currently can't think of any other way this could break... but it does seem quite surprising. Should we at least warn against inline(always) on closures inside target feature functions? I think we should.

A little context/summary here: the target feature attribute is only being dropped in codegen, at the language level it is present and it should still be accounted for when doing safety checks etc. #[inline(always)] and #[target_feature] don't play nice together in any context, because LLVM will refuse to inline mismatched target features and rustc will emit an ICE. Prior to tf1.1, I believe #[target_feature] wasn't propagated into closures at all--it only became relevant when making safe calls with tf1.1. Rust currently rejects #[target_feature] and #[inline(always)] on regular functions for this reason, but obviously we don't want to break any existing code by rejecting it on closures.

The assumption made is that if you want #[inline(always)] on a closure, it's being inlined into its enclosing function (otherwise the inline attribute is useless), so it will still inherit the target features via inlining. However, in the off chance you pass the closure in some way that results in a target feature mismatch, dropping the #[target_feature] attribute prevents the ICE from LLVM. Across an edition change we might be able to add an additional check that errors nicely if the #[inline(always)] closure escapes in that way, and allow inheriting the codegen target feature attribute.

calebzulawski · 2024-11-23T17:13:10Z

A possible alternative is to drop #[inline(always)] down to #[inline] in those scenarios, but I think that's more likely to break existing code that may have depended on that behavior to performance.

RalfJung · 2024-11-23T17:13:27Z

The end result of all that is confusing though - e.g. if inside the closure you call a tf 1.1 fn that needs a particular feature, that will not work (I assume). If you call an extern C fn that takes a vector, it will trigger the ABI mismatch lint. Hence the suggestion to inform the programmer that their inline(always) has some quite surprising consequences.

calebzulawski · 2024-11-23T17:19:48Z

It does work, because the target feature attribute is still present everywhere except codegen/LLVM: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=987b48c66d3ef5d7a0dcdf8f716d976c

You're right that the ABI mismatch lint was never accounted for in this solution, though. I think a lint is reasonable, hopefully across an edition change we can simply prevent the closure from being called in one of those edge cases and reenable the codegen attribute.

RalfJung · 2024-11-23T17:22:24Z

Also, the target feature inheritance itself is done in an odd way: just enabling the target_feature_11 feature, without changing anything else about the code, will change which target features are enabled in some closures. Generally, enabling a feature gate itself should never change anything about how we build the program, it should only unlock new syntax that then can change behavior.

It does work, because the target feature attribute is still present everywhere except codegen/LLVM: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=987b48c66d3ef5d7a0dcdf8f716d976c

This example doesn't show anything since the closure is not inline(always). But it seems to work even with an inline(always) closure.

hopefully across an edition change we can simply prevent the closure from being called in one of those edge cases and reenable the codegen attribute.

I don't think we should do such checks on how the closure is called, that's too complicated and confusing.

We should instead ensure our backend doesn't have critical soundness bugs like #116573 that we have to work around on the language level... forbidding inline(always) on closures in target_feature functions until then is reasonable IMO, just like we forbid it on target_feature functions themselves.

RalfJung · 2024-11-23T17:29:47Z

A possible alternative is to drop #[inline(always)] down to #[inline] in those scenarios, but I think that's more likely to break existing code that may have depended on that behavior to performance.

Not having the target feature in the closure can also be pretty bad for performance, can't it? Presumably people enabled the target feature for a reason.

I think we should have a lint that suggests people drop the always, and telling them that until they do that, the closure will not have the target feature enabled. It seems unlikely to me that this will be a worse problem than forbidding inline(always) on target_feature functions in general.

calebzulawski · 2024-11-23T17:37:29Z

A possible alternative is to drop #[inline(always)] down to #[inline] in those scenarios, but I think that's more likely to break existing code that may have depended on that behavior to performance.

Not having the target feature in the closure can also be pretty bad for performance, can't it? Presumably people enabled the target feature for a reason.

Prior to target_feature_11, the target features weren't enabled on any closure (see the LLVM IR): https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=cc8f5a52cd484f6ee54b7457215fd945

I support adding the lint, as long as you are still permitted to use #[inline(always)] to inline into the enclosing function if that's what you really want (user's choice to disable the lint).

RalfJung · 2024-11-23T17:40:33Z

I support adding the lint, as long as you are still permitted to use #[inline(always)] to inline into the enclosing function if that's what you really want (user's choice to disable the lint).

That seems odd to me; we don't allow this for regular functions either.

Do you have a concrete usecase where this is relevant? I don't think we should just go based on "vibes" here. We'd have a future-compat lint for a while, and if nobody shows up with a usecase, IMO we should make it a hard error.

calebzulawski · 2024-11-23T17:46:12Z

Oh, I'm fine with that plan as well, it's the most consistent. #108655 suggested to me that there were users who wanted it, I don't personally have an example. However it does seem there's a PR coming to allow #[inline(always)] on regular target feature functions by checking that there isn't a target feature mismatch at the call site, I'm not sure how successful that will be.

RalfJung · 2024-11-23T17:57:02Z

However it does seem there's a PR coming to allow #[inline(always)] on regular target feature functions by checking that there isn't a target feature mismatch at the call site, I'm not sure how successful that will be.

I heard there are such plans but IMO they should be blocked on fixing LLVM. Since I posted the link to the soundness issue in that Zulip thread, there has not been further discussion there AFAIK. But anyway that is a separate discussion.

hanna-kruppe · 2024-11-24T10:00:23Z

Do you have a concrete usecase where this is relevant? I don't think we should just go based on "vibes" here. We'd have a future-compat lint for a while, and if nobody shows up with a usecase, IMO we should make it a hard error.

I've written some code which I think would trigger the future-compat lint you're proposing (obviously I can't try it out yet). I think it's reasonable code:

There's an inline(always) higher order function involved because I want to share common code structure between multiple implementations of the same algorithm. Currently that's five implementations total (including a portable non-SIMD one) and I could add even more once Rust stabilizes SIMD intrinsics for other platforms. The alternative would be a macro, but that's clearly worse.
I'm doing dynamic feature detection for AVX2, so I need to use #[target_feature(enable = ...)] on the outer function. I'm passing a closure because I'm using a witness with safety invariant "AVX2 is available" to make the AVX2 intrinsics safe-to-use, so the closure has a capture (even if it's ZST). I could manually desugar the closure into a free function with an extra parameter, but that would complicate eight_rounds and every backend that doesn't do dynamic feature detection.
The closure has to be inline(always) because otherwise I've seen the calls to it not getting inlined, and that's fatal for performance. I'm not sure if the closure having different target features has anything to do with it or not, but even if making closures inherit them would help in this specific case, there will always be cases where the programmer wants to tell LLVM to inline harder. Plus, libraries can't rely on this in their design decisions until they drop support for Rust versions prior to that change.

There are ways I could rework my code to side-step the lint, but all I can think of make the code more ugly and brittle. And I don't really see a reason why I should have to do this. As I understand it, there's no problem with my code either under the old (closure doesn't inherit target_feature) or proposed new (does inherit) semantics. I appreciate that passing such a closure outside of the function it's defined in causes problems when combined with target_feature inheritance and LLVM's current behavior. But I don't think a future-compat warning and eventually hard error on perfectly good code is a nice way to handle that.

RalfJung · 2024-11-24T10:10:53Z

@hanna-kruppe thanks! Yes that would trigger the lint.

there will always be cases where the programmer wants to tell LLVM to inline harder.

Yes, but that's just incompatible with per-function target features, until LLVM fixes that bug.

So the question is, what's more important -- having the target features in the closure, or having inline(always)?

Would be interesting whether inheriting target features and regular #[inline] are sufficient to get the desired result here.

As I understand it, there's no problem with my code either under the old (closure doesn't inherit target_feature) or proposed new (does inherit) semantics.

To be clear, the new semantics is that your closure still does not inherit target features, because it is inline(always).

hanna-kruppe · 2024-11-24T10:38:01Z

In code like this, I mostly treat closures and non-target_feature helper functions as a more convenient alternative to macros and manual copy-paste: useful for structuring my source code, but intended to be melted down into a single big function. In most cases I don't think it matters which way LLVM takes to get there (always-inline or regular inline) because once the helper functions are inlined they'll be in a function with the right target features anyway. But I suppose inline(always) reflects my intentions more precise in extreme cases and for opt-level = "s" builds, which doesn't always handle the "write nice, small composable functions and hope LLVM inlines them all" style of programming very well.

Of course there are scenarios where I don't want or need everything to be inlined, e.g. because it's a big chunk of code that gets called from several different places. But in those cases I'm not slapping inline(always) on it, and it's less likely to be a closure in the first place.

RalfJung · 2024-11-24T10:45:07Z

I mean we could also say that we'll "just" document that target_features is inherited into closures except for inline(always) closures. I just thought that'd be too surprising, and people would just assume it gets always inherited.

veluca93 · 2024-11-24T10:46:43Z

I mean we could also say that we'll "just" document that target_features is inherited into closures except for inline(always) closures. I just thought that'd be too surprising, and people would just assume it gets always inherited.

Note that the current behaviour is somewhat more surprising than that: it is inherited in inline(always) closures for purposes of safety checking, but not for codegen...

hanna-kruppe · 2024-11-24T10:58:26Z

What difference does not inheriting make, anyway?

For safety checking: not inheriting would mean you'd still need unsafe to call any target_feature functions in closures, even if the call would be safe in the enclosing function? Sounds pretty annoying. Edit: and I don't think the humble #[inline] attribute should be allowed to have such a big semantic impact.
For codegen: I think it only matters in cases where the inline(always) closure isn't inlined after all. Or I guess if it's inlined into a caller that lacks the target features the closure would have inherited. Both seems like pretty niche problems to have.

RalfJung · 2024-11-24T11:21:14Z

If we do inherit target features into inline(always) closures for codegen, I can construct a miscompilation based on #116573. I think that's a blocker.

hanna-kruppe · 2024-11-24T11:56:46Z

I hadn't seen that issue before and got confused trying to match what you're saying to the original reproducer in that issue. Are you saying inline(always) closure with target features inherited for codegen would give you the gadget for bypassing the check from #127731 described in #116573 (comment) ?

[...] we need a "with target feature" function and coerce LLVM into inlining its call site into a "no target feature" function. It will only do that when the "with target feature" function has inline(always), [...]

RalfJung · 2024-11-24T12:05:14Z

Yes that is exactly what I am saying. Specifically the example I posted above would be exactly that gadget, and I am fairly sure it would miscompile.

calebzulawski · 2024-11-24T19:07:14Z

I've written some code which I think would trigger the future-compat lint you're proposing (obviously I can't try it out yet).

This looks like it would trigger it. While not directly related to this topic, I see you are passing the closure via Fn--I've run into issues passing closures via Fn* traits in the past (#96929)

RalfJung · 2024-11-25T07:10:52Z

The idea that target_features inhibits inlining or that inlining inhibits target feature usage doesn't make sense.

It's a work-around for an LLVM bug. In an ideal world we wouldn't have to worry about this.

why we can't inline a function f() with target features into a function g() that doesn't have a superset of f() target features

Strictly speaking this doesn't make sense either. LLVM "just" has to ensure that the operations from f are not hoisted out of any conditionals or loops in g. But LLVM isn't able to represent that, so instead it just stops inlining altogether. Which then puts us into a pickle regarding inline(always) (even ignoring the bug): the user requested to be always inlined, but we can't actually make that happen if the function has target features. What do we do?

The status quo is that:

on regular functions, we just error on that combination
on a closure, then if TF 1.1 is turned on we'd usually inherit the surrounding function's target features into the closure, but if the closure is inline(always) then we don't inherit

It's not pretty, but between that long-standing LLVM bug and the general LLVM limitation around inlining and target features, and given our backwards compatibility constraints, it is unclear how to do better.

Compared to stable, with TF 1.1 nothing changes for the behavior of inline(always) functions or closures. What does change is the behavior of non-inline(always) closures in a target_feature function.

nikomatsakis mentioned this issue Feb 12, 2020

target_feature 1.1 rust-lang/rfcs#2396

Merged

nikomatsakis added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 12, 2020

rustbot self-assigned this Feb 18, 2020

LeSeulArtichaut mentioned this issue Feb 18, 2020

Implement RFC 2396: #[target_feature] 1.1 #69274

Merged

rustbot removed their assignment May 3, 2020

hanna-kruppe mentioned this issue May 8, 2020

target_feature_11 allows bypassing safety checks through Fn* traits #72012

Closed

nikomatsakis added the F-target_feature_11 target feature 1.1 RFC label May 8, 2020

LeSeulArtichaut added B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. and removed B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. labels Oct 22, 2020

nikomatsakis mentioned this issue May 21, 2021

Add DerefOfRawPointer and CallToFunctionWith to THIR unsafeck #85306

Closed

lu-zero mentioned this issue May 23, 2021

Leverage target_feature 1.1 xiph/rav1e#1255

Open

LeSeulArtichaut mentioned this issue Mar 17, 2022

Document the target_feature_11 feature rust-lang/reference#1181

Open

joshtriplett added the S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR label Jun 29, 2022

LeSeulArtichaut mentioned this issue Jul 26, 2022

Stabilize #![feature(target_feature_11)] #99767

Merged

veluca93 mentioned this issue Nov 23, 2024

Safe intrinsics in core::arch rust-lang/libs-team#494

Closed

calebzulawski mentioned this issue Nov 24, 2024

Lint #[inline(always)] closure in #[target_feature] functions #133408

Closed

This comment has been minimized.

Sign in to view

veluca93 mentioned this issue Dec 9, 2024

Stabilize target_feature_11 #134090

Open

Tracking issue for target_feature 1.1 RFC #69098

Tracking issue for target_feature 1.1 RFC #69098

Comments

nikomatsakis commented Feb 12, 2020 • edited by RalfJung Loading

People

Step

Unresolved questions

LeSeulArtichaut commented Feb 18, 2020

hanna-kruppe commented May 2, 2020

petrochenkov commented May 2, 2020 • edited Loading

LeSeulArtichaut commented May 3, 2020

LeSeulArtichaut commented May 3, 2020

hanna-kruppe commented May 8, 2020

nikomatsakis commented Jun 22, 2020

calebzulawski commented Aug 22, 2021

nikomatsakis commented Mar 16, 2022

LeSeulArtichaut commented Mar 17, 2022

RalfJung commented Nov 23, 2024

RalfJung commented Nov 23, 2024 • edited Loading

calebzulawski commented Nov 23, 2024

calebzulawski commented Nov 23, 2024

RalfJung commented Nov 23, 2024 via email • edited Loading

calebzulawski commented Nov 23, 2024

RalfJung commented Nov 23, 2024 • edited Loading

RalfJung commented Nov 23, 2024

calebzulawski commented Nov 23, 2024

RalfJung commented Nov 23, 2024

calebzulawski commented Nov 23, 2024

RalfJung commented Nov 23, 2024 via email

hanna-kruppe commented Nov 24, 2024 • edited Loading

RalfJung commented Nov 24, 2024

hanna-kruppe commented Nov 24, 2024 • edited Loading

RalfJung commented Nov 24, 2024

veluca93 commented Nov 24, 2024

hanna-kruppe commented Nov 24, 2024 • edited Loading

RalfJung commented Nov 24, 2024

hanna-kruppe commented Nov 24, 2024

RalfJung commented Nov 24, 2024

This comment has been minimized.

This comment has been minimized.

calebzulawski commented Nov 24, 2024

This comment has been minimized.

This comment has been minimized.

RalfJung commented Nov 25, 2024 • edited Loading

nikomatsakis commented Feb 12, 2020 •

edited by RalfJung

Loading

petrochenkov commented May 2, 2020 •

edited

Loading

RalfJung commented Nov 23, 2024 •

edited

Loading

RalfJung commented Nov 23, 2024 via email •

edited

Loading

RalfJung commented Nov 23, 2024 •

edited

Loading

hanna-kruppe commented Nov 24, 2024 •

edited

Loading

hanna-kruppe commented Nov 24, 2024 •

edited

Loading

hanna-kruppe commented Nov 24, 2024 •

edited

Loading

RalfJung commented Nov 25, 2024 •

edited

Loading