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 #2396

Merged
merged 18 commits into from
Feb 12, 2020
Merged
241 changes: 241 additions & 0 deletions text/0000-target-feature-1.1.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
- Feature Name: `#[target_feature]` 1.1
- Start Date: 2018-04-06
- RFC PR: (leave this empty)
- Rust Issue: (leave this empty)

# Summary
[summary]: #summary

This RFC attempts to resolve some of the unresolved questions in [RFC 2045
(`target_feature`)]. In particular, it allows:

* specifying `#[target_feature]` functions without making them `unsafe fn`
* calling `#[target_feature]` functions in some contexts without `unsafe { }` blocks

It achieves this by proposing three incremental steps that we can sequentially
make to improve the ergonomics and the safety of target-specific functionality
without adding run-time overhead.

[RFC 2045 (`target_feature`)]: https://github.com/rust-lang/rfcs/pull/2045

# Motivation
[motivation]: #motivation

> This is a brief recap of [RFC 2045 (`target_feature`)].

The `#[target_feature]` attribute allows Rust to generate machine code for a
function under the assumption that the hardware where the function will be
executed on supports some specific "features".

If the hardware does not support the features, the machine code was generated
under assumptions that do not hold, and the behavior of executing the function
is undefined.

[RFC 2045 (`target_feature`)] guarantees safety by requiring all
`#[target_feature]` functions to be `unsafe fn`, thus preventing them from being
called from safe code. That is, users have to open an `unsafe { }` block to call
these functions, and they have to manually ensure that their pre-conditions
hold - for example, that they will only be executed on the appropriate hardware
by doing run-time feature detection, or using conditional compilation.

And that's it. That's all [RFC 2045 (`target_feature`)] had to say about this.
Back then, there were many other problems that needed to be solved for all of
this to be minially useful, and [RFC 2045 (`target_feature`)] dealt with those.
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved

However, the consensus back then was that this is far from ideal for many
reasons:

* when calling `#[target_feature]` functions from other `#[target_feature]`
functions with the same features, the calls are currently still `unsafe` but
they are actually safe to call.
* making all `#[target_feature]` functions `unsafe fn`s and requiring `unsafe
{}` to call them everywhere hides other potential sources of `unsafe` within
these functions. Users get used to upholding `#[target_feature]`-related
Comment on lines +51 to +53
Copy link
Member

Choose a reason for hiding this comment

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

Related RFC: #2585

But the proposal here still makes sense regardless of that.

pre-conditions, and other types of pre-conditions get glossed by.
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved
* `#[target_feature]` functions are not inlined across mismatching contexts,
which can have disastrous performance implications. Currently calling
`#[target_feature]` function from all contexts looks identical which makes it
easy for users to make these mistakes (which get reported often).

The solution proposed in this RFC solves these problems.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

Currently, we require that `#[target_feature]` functions be declared as `unsafe
fn`. This RFC relaxes this restriction:

* safe `#[target_feature]` functions can be called _without_ an `unsafe {}`
block _only_ from functions that have at least the exact same set of
`#[target_feature]`s. Calling them from other contexts (other functions, static
variable initializers, etc.) requires opening an `unsafe {}` even though they
are not marked as `unsafe`:

```rust
// Example 1:
#[target_feature = "sse2"] unsafe fn foo() { } // RFC2045
#[target_feature = "sse2"] fn bar() { } // NEW
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved

// This function does not have the "sse2" target feature:
fn meow() {
foo(); // ERROR (unsafe block required)
unsafe { foo() }; // OK
bar(); // ERROR (meow is not sse2)
unsafe { bar() }; // OK
}

#[target_feature = "sse2"]
fn bark() {
foo(); // ERROR (foo is unsafe: unsafe block required)
unsafe { foo() }; // OK
bar(); // OK (bark is sse2 and bar is safe)
unsafe { bar() }; // OK (as well - warning: unnecessary unsafe block)
}

#[target_feature = "avx"] // avx != sse2
fn moo() {
foo(); // ERROR (unsafe block required)
unsafe { foo() }; // OK
bar(); // ERROR (moo is not sse2 but bar requires it)
unsafe { bar() }; // OK
}
```

> Note: while it is safe to call an SSE2 function from _some_ AVX functions,
> this would require specifying how features relate to each other in
> hierarchies. It is unclear whether those hierarchies actually exist, but
> adding them to this RFC wouldunnecessary complicate it and can be done
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved
> later or in parallel to this one, once we agree on the fundamentals.

First, this is still sound. The caller has a super-set of `#[target_features]`
of the callee. That is, the `#[target_feature]`-related pre-conditions of the
callee are uphold by the caller, therefore calling the callee is safe.

This change already solves all three issues mentioned in the motivation:

* When calling `#[target_feature]` functions from other `#[target_feature]`
functions with the same features, we don't need `unsafe` code anymore.
* Since `#[target_feature]` functions do not need to be `unsafe` anymore,
`#[target_feature]` functions that are marked with `unsafe` become more
visible, making it harder for users to oversee that there are other
pre-conditions that must be uphold.
* `#[target_feature]` function calls across mismatching contexts require
`unsafe`, making them more visible. This makes it easier to identify
calls-sites across which they cannot be inlined while making call-sites across
which they can be inlined more ergonomic to write.

The `#[target_feature]` attribute continues to not be allowed on safe trait
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved
method implementations:

```rust
// Example 2:
trait Foo { fn foo(); }
struct Fooish();
impl Foo for Fooish {
#[target_feature = "sse2"] fn foo() { }
// ^ ERROR: #[target_feature] on trait method impl requires
// unsafe fn but Foo::foo is safe
// (this is already an error per RFC2045)
}

trait Bar { unsafe fn bar(); }
struct Barish();
impl Bar for Barish {
#[target_feature = "sse2"] unsafe fn bar() { } // OK (RFC2045)
}
```

* safe `#[target_feature]` functions are not assignable to safe `fn` pointers.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unfortunate. However, it is fixable by allowing attributes on types syntactically as proposed in #2602. Can you note this as a future possibility? (We have forward compat with this, so it doesn't need to be part of the proposal right now.)

Copy link
Contributor Author

@gnzlbg gnzlbg Nov 27, 2019

Choose a reason for hiding this comment

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

It's not just a matter of syntax, but also semantics, since the attribute would need to be part of the function type, figure out coercions if any, be able to impl Trait for #[target_feature(eanble = "avx")] fn(...) -> ... { ... }, etc.

And well, in general, the coercion of a #[target_feature] fn pointer to a bare fn is unsound, so any kind of future extension would need to solve that problem, but AFAIK nobody has proposed a solution to it.

Copy link
Contributor Author

@gnzlbg gnzlbg Nov 27, 2019

Choose a reason for hiding this comment

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

I've added a note, but I'm not sure what you are asking can work.

You are basically asking for, given a trait Foo with safe and unsafe methods, a way to implement the safe methods as unsafe fns such that code is parametric over that (e.g. because target-feature makes a fn unsafe). This is trivial if all methods in the trait are unsafe, but if there is one safe method, Foo could be object safe, and calling its methods would be safe, so I have no idea how we could make an unsafe version of it sound through that safe API.

The realization is that being parametric over target feature is like being parametric over unsafe:

trait Foo { fn foo(&self); } // object safe
impl Foo for Bar { unsafe fn foo(&self) { ... } }

fn baz(x: &dyn Foo) { x.foo(); /* unsound * / }

So AFAICT, impl #[target_feature] Trait can only work if all Trait methods are unsafe, and even then, notice how baz does not have a #[target_feature] attribute, meaning that the call to unsafe { x.foo() } has little chances of "proving" whether safety is provided, since that would depend on the target features, and these are not specified anywhere.



```rust
// Example 3
#[target_feature] fn meow() {}

static x: fn () -> () = meow;
// ^ ERROR: meow can only be assigned to unsafe fn pointers due to
// #[target_feature] but function pointer x with type fn()->() is safe.
static y: unsafe fn () -> () = meow as unsafe fn()->(); // OK
```

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

This RFC proposes to changes to the language with respect to [RFC 2045 (`target_feature`)]:

* safe `#[target_feature]` functions can be called _without_ an `unsafe {}`
block _only_ from functions that have at least the exact same set of
`#[target_feature]`s. Calling them from other contexts (other functions, static
variable initializers, etc.) requires opening an `unsafe {}` even though they
are not marked as `unsafe`

* safe `#[target_feature]` functions are not assignable to safe `fn` pointers.

# Drawbacks
[drawbacks]: #drawbacks

This RFC extends the typing rules for `#[target_feature]`, which might
unnecessarily complicate future language features like an effect system.

# Rationale and alternatives
[alternatives]: #alternatives

Since `#[target_feature]` are effects or restrictions (depending on whether we
`enable` or `disable` them), the alternative would be to integrate them with an
effect system. Since `#[target_feature]` is already on its path to stabilization
and its required to make SIMD in Rust minimally useful, not stabilizing
`#[target_feature]` at this point is not an option unless we are willing to
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved
delay SIMD until an effect system is stabilized. Until then, `#[target_feature]`
and this RFC solve real problems and provide another use-case that such an
effect system will need to enable to be useful for Rust programmers.

# Prior art
[prior-art]: #prior-art

[RFC2212 target feature unsafe](https://github.com/rust-lang/rfcs/pull/2212)
attempted to solve this problem. This RFC builds on the discussion that was
produced by that RFC and by many discussions in the `stdsimd` repo.

# Unresolved questions
[unresolved]: #unresolved-questions

## Negative features

[RFC 2045 (`target_feature`)] introduced the `#[target_feature(enable = "x")]`
syntax to allow introducing negative features in future RFCs in the form of
`#[target_feature(disable = "y")]`. Since these have not been introduced yet we
can only speculate about how they would interact with the extensions proposed in
this RFC but we probably can make the following work in some form:

```rust
// #[target_feature(enable = "sse")]
fn foo() {}

#[target_feature(disable = "sse")]
fn bar() {
foo(); // ERROR: (bar is not sse)
unsafe { foo() }; // OK
}

fn baz() {
bar(); // OK
}
```

## Effect system

It is unclear how `#[target_feature]` would interact with an effect system for
Rust like the one being tracked
[here](https://github.com/Centril/rfc-effects/issues) and discussed in
[RFC2237](https://github.com/rust-lang/rfcs/pull/2237).

In particular, it is unclear how the typing rules being proposed here would be
covered by such an effect system, and whether such system would support
attributes in effect/restriction position.

Such an effect-system might need to introduce first-class target-features into
the language (beyond just a simple attribute) which could lead to the
deprecation of the `#[target_feature]` attribute.

It is also unclear how any of this interacts with effect-polymorphism at this
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved
point.