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

Tracking issue for target_feature 1.1 RFC #69098

Open
7 of 10 tasks
nikomatsakis opened this issue Feb 12, 2020 · 46 comments
Open
7 of 10 tasks

Tracking issue for target_feature 1.1 RFC #69098

nikomatsakis opened this issue Feb 12, 2020 · 46 comments
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-target_feature_11 target feature 1.1 RFC S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 12, 2020

This is a tracking issue for the RFC "target_feature 1.1" (rust-lang/rfcs#2396).

Issues: F-target_feature_11 target feature 1.1 RFC

People

Last updated in Mar 2023:

  • Shepherd: gnzlbg @workingjubilee (person who can help answer tricky questions that arise during implementation)
  • Lang team liaison: @joshtriplett (main point of contact from lang team)

Step

Unresolved questions

@nikomatsakis 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
@LeSeulArtichaut
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

petrochenkov commented May 2, 2020

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.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 2, 2020
…r=hanna-kruppe

Implement RFC 2396: `#[target_feature]` 1.1

Tracking issue: rust-lang#69098

r? @nikomatsakis
cc @gnzlbg @joshtriplett
bors added a commit to rust-lang-ci/rust that referenced this issue May 2, 2020
…hanna-kruppe

Implement RFC 2396: `#[target_feature]` 1.1

Tracking issue: rust-lang#69098

r? @nikomatsakis
cc @gnzlbg @joshtriplett
@LeSeulArtichaut
Copy link
Contributor

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

@LeSeulArtichaut
Copy link
Contributor

@rustbot release-assignment

@hanna-kruppe
Copy link
Contributor

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

@nikomatsakis nikomatsakis added the F-target_feature_11 target feature 1.1 RFC label May 8, 2020
@nikomatsakis
Copy link
Contributor Author

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

@LeSeulArtichaut 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
@calebzulawski
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
}

@joshtriplett 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
compiler-errors added a commit to compiler-errors/rust that referenced this issue Feb 26, 2023
…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
Copy link
Member

The two remaining bugs listed here now have mitigations:

There is one bug that carries the label, #58279, but I think that one can be closed.

So... seems like there's no longer a blocker here?

@RalfJung
Copy link
Member

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
Copy link
Member

RalfJung commented Nov 23, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

RalfJung commented Nov 23, 2024 via email

@calebzulawski
Copy link
Member

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
Copy link
Member

RalfJung commented Nov 23, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

RalfJung commented Nov 23, 2024 via email

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Nov 24, 2024

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:

  1. 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.
  2. 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.
  3. 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
Copy link
Member

@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
Copy link
Contributor

hanna-kruppe commented Nov 24, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

hanna-kruppe commented Nov 24, 2024

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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.

@briansmith

This comment has been minimized.

@calebzulawski

This comment has been minimized.

@calebzulawski
Copy link
Member

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)

@briansmith

This comment has been minimized.

@calebzulawski

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Nov 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC F-target_feature_11 target feature 1.1 RFC S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests