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

target-feature 1.1: should closures inherit target-feature annotations? #73631

Closed
nikomatsakis opened this issue Jun 22, 2020 · 12 comments · Fixed by #78231
Closed

target-feature 1.1: should closures inherit target-feature annotations? #73631

nikomatsakis opened this issue Jun 22, 2020 · 12 comments · Fixed by #78231
Assignees
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-target_feature_11 target feature 1.1 RFC finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

This example currently gives an error (playground):

#![feature(target_feature_11)]

#[target_feature(enable="avx")]
fn also_use_avx() {
    println!("Hello from AVX")
}

#[target_feature(enable="avx")]
fn use_avx() -> Box<dyn Fn()> {
    Box::new(|| also_use_avx())
}

fn main() {
}

Should it? @joshtriplett made the case that, once you enter use_avx, you have demonstrated that the target feature is present on your system, so it would make sense for closures declared within that item to "inherit" the target features from their enclosing function items (i.e., be able to invoke target-feature functions that require them, and probably also get the attributes declared within LLVM).

Some questions:

  • Is there some notion of "scoped" target features? i.e., as the example demonstrates, this allows target-feature functions to "escape" into the wider world, is that a problem?
  • If closures do inherit, should they still be considered to implement FnOnce etc? (See target_feature_11 allows bypassing safety checks through Fn* traits #72012)
  • Would we want the ability to explicitly annotate closures with #[target_feature(...)]? If so, that would presumably mean that explicitly annotated closures cannot implement FnOnce, which might be surprising and non-obvious.
@nikomatsakis nikomatsakis added F-target_feature_11 target feature 1.1 RFC T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jun 22, 2020
@nikomatsakis
Copy link
Contributor Author

cc @hanna-kruppe, @petrochenkov

@calebzulawski
Copy link
Member

I have run into boxed closures performing poorly, it isn't obvious that something like this would be exceptionally slow--orders of magnitude worse than the naive version since each intrinsic results in a callq:

#[target_feature(enable = "avx")]
fn add_op(value: f32) -> Box<dyn Fn(&mut [f32])> {
    use std::arch::x86_64::*;
    Box::new(move |x: &mut [f32]| {
        for s in x.chunks_mut(8) {
            unsafe {
                _mm256_storeu_ps(
                    s.as_mut_ptr(),
                    _mm256_add_ps(_mm256_loadu_ps(s.as_ptr()), _mm256_set1_ps(value)),
                );
            }
        }
    })
}

Not related to closures, but inheriting features, should creating function pointers be allowed as well? For example:

#[target_feature(enable = "avx")]
fn foo() {}

#[target_feature(enable = "avx")]
fn bar() -> fn() {
    foo
}

@nikomatsakis
Copy link
Contributor Author

@calebzulawski I think any performance implications of boxing are pretty orthogonal to the question at hand.

The question of coercing to a fn pointer, though, is pretty germane. I think that should probably be supported, yes, by the same logic.

(I guess it would be unsafe to do outside of a suitable target-feature function?)

@calebzulawski
Copy link
Member

Sorry if I was unclear--I was referring to the fact that the intrinsics fail to inline into the closure despite it appearing within a target-feature function, unrelated to the implication of boxing the closure. Pre-target-feature-1.1 this is particularly non-obvious because the unsafe block is always required and there is no indication that the closure isn't inheriting the features.

@nikomatsakis
Copy link
Contributor Author

@rfcbot fcp merge

Having heard no objections, I'd like to call this to question. Dear @rust-lang/lang I propose that we make the change described in the OP for the reasons that were outlined there.

@rfcbot
Copy link

rfcbot commented Jul 1, 2020

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 1, 2020
@nikomatsakis
Copy link
Contributor Author

Discussed this in our @rust-lang/lang meeting today. Notes from our minutes:

  • Assumption that this makes is that once you run code with a given target feature, you will always have that target feature available.
  • In theory you could have some complex architecture where you have asymmetric multiprocessing that has distinct capabilities:
    • But this would break a huge amount of code in the wild that assumes you can check the CPUID once and then cache that result.
    • There has been discussion in the linux community that in such a scenario, you would get back the universal subset available, and would have to “opt in” to observing features that are specific to your sort of core.
  • New type of UB: “taking a target feature away from your process after having run code that uses that target feature is UB”.
    • We could change this in the future if we need to deal with complex asymmetric multiprocessing scenarios as described above.
  • Definition of the unsafe proof obligation to call a target-feature:
    • You must show that the target-feature is available while the function executes and whatever may escape from that function.
    • This emphasized condition is trivial in the case where cpu features never change.
    • In a hybrid architecture where cpu features can change, you would have to know whether fn and closure pointers escape and for how long. This may imply new annotations that assure you that they do not escape or perhaps adding a lifetime annotation.
      • Lifetime annotations could also be useful for dynamic loading/unloading scenarios where functions get unloaded.

@nikomatsakis
Copy link
Contributor Author

Ping @cramertj @pnkfelix and @withoutboats -- checkbox

@dbdr
Copy link

dbdr commented Sep 1, 2020

I was also expecting inner functions to inherit the target-feature annotations of the outer function. Can this question fit here or does it warrant a separate issue?

@workingjubilee
Copy link
Member

workingjubilee commented Sep 12, 2020

I'm sorry, I'm not sure I understand everything at hand, but would this not implicitly resolve #64609 question about the Rust calling convention / ABI in favor of yes? Or at least move it much closer to, thus bringing a soundness hole closer to resolution?

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 15, 2020
@rfcbot
Copy link

rfcbot commented Sep 15, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 25, 2020
@rfcbot
Copy link

rfcbot commented Sep 25, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Sep 25, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Oct 1, 2020
@LeSeulArtichaut LeSeulArtichaut self-assigned this Oct 22, 2020
@bors bors closed this as completed in 00c4dcd Oct 23, 2020
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`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 27, 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``
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 28, 2023
…-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`
@workingjubilee workingjubilee added the A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. label Mar 3, 2023
saethlin pushed a commit to saethlin/miri that referenced this issue Mar 5, 2023
…tebank

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/rust#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 #69098
r? `@ghost`
thomcc pushed a commit to tcdi/postgrestd that referenced this issue May 31, 2023
…tebank

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/rust#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 #69098
r? `@ghost`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
…tebank

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/rust#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 #69098
r? `@ghost`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…tebank

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/rust#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 #69098
r? `@ghost`
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. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-target_feature_11 target feature 1.1 RFC finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants