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

target_feature 1.1 #2396

merged 18 commits into from Feb 12, 2020

Conversation

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 6, 2018

Rendered

@gnzlbg gnzlbg force-pushed the gnzlbg:tf11 branch from f79d0f0 to 76c213b Apr 6, 2018
@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 6, 2018

This looks good to me 👍. It's a nice conservative "obviously sound" next stop.

I talked a little bit to @gnzlbg about the interaction with the portability lint and target_feature in general; which @gnzlbg says hasn't really been discussed. Thankfully, static #[target_feature ..] and #[cfg(target_feature ...)] should be a shoe-in allowing us to go from a simple equivalence relation (equal set of annotations) to a richer lattice, continuing the trend this RFC establishes. cfg!(target_feature..) is more interesting...

@Centril Centril added the T-lang label Apr 6, 2018
@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Apr 7, 2018

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Apr 7, 2018

@newpavlov posted a pre-RFC about target restriction contexts: https://internals.rust-lang.org/t/pre-pre-rfc-target-restriction-contexts/7163/

I don't think this RFC is incompatible with that proposal, but that's something worth triple-checking since those are things that we might want to do later anyways.

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Apr 7, 2018

Beautiful RFC, big 👍 .

One small point: You probably already know it, but to one important thing for soundness is probably that #[target_feature] attributes should influence the type of the function during function pointer creation. In code:

#[target_feature = "sse2"] fn bar() { }

fn test() {
    let a: fn() = bar; // ERROR mismatched types expected fn() got #[target_feature ="sse2"] fn()
    a();

    let b: #[target_feature] fn() = bar; // works
    b();
}

To get the feature onto stable faster (syntax bikesheds are tiring), we could not implement the let b: #[target_feature] fn() = bar; feature initially, but the type mismatch is important.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Apr 7, 2018

@est31

one important thing for soundness is probably that #[target_feature] attributes should influence the type of the function during function pointer creation

So right now (RFC2045) you can't have a safe function pointer to a #[target_feature] function - this RFC does not change that. That is shown in the "Example 3". Basically one needs to use an unsafe function pointer and coerce the #[target_feature] function into it. The user of the function pointer needs to open an unsafe block and make sure that any pre-conditions are uphold, so AFAICT that is sound.

This restriction can be relaxed later, but doing so adds some complexity, and I don't think that it buys us that much more, so I wouldn't want to delay anything here for that.

The reason it doesn't buy us that much is that today one can already write safe wrappers around unsafe function pointers. The wrapper ensures that if you construct it properly, the function pointer is only set to a function that is actually safe to call, for example, by doing run-time or compile-time feature detection.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 7, 2018

@est31 yeah adding annotations to the type system like that is kind of a big deal. OTOH we can achieve that sort of thing with the portability lint (see rust-lang-nursery/portability-wg#17 for details). In general [i.e with enough squinting :D], the portability lint can be viewed as a sort of overlay effect system with subtyping, so it's perfect for this sort of thing.

@hanna-kruppe

This comment has been minimized.

Copy link
Member

hanna-kruppe commented Apr 7, 2018

But the portability lint isn't sound, right? The solution for mismatched target features has to be sound because calling a function with the wrong target features is UB.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 8, 2018

@rkruppe the portability lint is only not sound because we choose to make it warn not err. I guess I got ahead of myself thinking of a future epoch where it does raise errors :).

I'll back-pedal and say in the nearer term it would indeed be a separate feature as @est31 describes, but share the same implementation. Then in that future epoch we flip the switch and the features are unified.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 9, 2018

@gnzlbg thanks for the RFC! On reading I was having a difficult time figuring out the difference to #2212 (which is for now postponed), could you elaborate a bit on the differences?

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Apr 9, 2018

@alexcrichton

On reading I was having a difficult time figuring out the difference to #2212 (which is for now postponed), could you elaborate a bit on the differences?

I just read RFC2212 twice and I still don't think I fully understands what it proposes, so these are the differences that I identify (please correct me if I understood RFC2212 incorrectly):

  • RFC2212 introduces feature hierarchies, which allows safe #[target_feature] functions to be called from functions that have the feature enabled, but do not explicitly state so (e.g. because AVX2 implies AVX). This RFC is less powerful but simpler since it just requires the features to match exactly (e.g. AVX2 does not imply AVX). Introducing feature hierarchies is controversial and has to be done with care, it probably deserves its own RFC.
  • RFC2212 mentions that target features become part of a functions type, this RFC does not. I don't understand exactly how target features interact with the type-system in RFC2212 so I am not sure whether this is a big difference, or whether this matters at all or not.
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 9, 2018

Ok cool, thanks for the clarification! I would personally be on board with the construction in this RFC, I think it definitely makes sense!

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Apr 10, 2018

I guess this RFC needs a shepherd then :)

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Apr 10, 2018

I guess this RFC needs a shepherd then :)

I can do that =)

@parched

This comment has been minimized.

Copy link

parched commented Apr 11, 2018

What about

fn foo() { }

#[target_feature = "x"] fn bar() {
    foo(); // is an unsafe block required?
}

Am I right in assuming that no unsafe block is required here because currently we don't allow disabling features and have no negative features? In future when we add feature hierarchies then we can allow disabling features and negative features and the compiler can require a unsafe block here in those cases.

@hsivonen

This comment has been minimized.

Copy link

hsivonen commented Apr 11, 2018

AFAICT the substantive differences between RFC 2212 and this RFC are that

  1. this RFC doesn't define the hierarchy of higher SSE versions implying the lower ones
  2. this RFC requires the caller and the callee to have the exact same target_feature list for the call to be safe while RFC 2212 requires the callee to have a subset of of the caller's target_feature list (taking into account the features implied by the SSE hierarchy)
  3. RFC 2212 prohibits function pointers to safe target_feature functions but this RFC allows unsafe function pointers to such functions.

Correct?

while it is safe to call an SSE2 function from an AVX one, this would require specifying how features relate to each other in hierarchies. This would unnecessary complicate this RFC and can be done later once we agree on the fundamentals.

I think this is quite unfortunate given how the SSE family is organized. I think it would be beneficial to define the hierarchy for the SSE family even if defining other hierarchies was deferred. I can understand why there could exist controversial hierarchies (esp. AVX-512 family not being strictly additive), but it seems sad to refuse to define any hierarchies when the most common case is a well-known hierarchy.

Also, it seems unnecessarily strict to require the callee to have the same set of features as the caller, since logically a subset should suffice. Surely after this RFC functions with some target_feature are still allowed to call safe functions with no explicit target_feature declarations. That's already a subset relation anyway.

RFC2212 mentions that target features become part of a functions type, this RFC does not. I don't understand exactly how target features interact with the type-system in RFC2212 so I am not sure whether this is a big difference, or whether this matters at all or not.

This RFC needs some information about the functions for the compiler to decide that meow is not allowed to call bar but bark is. What that information is called is a matter of terminology, but I don't think the two RFCs differ in substance on this point.

In any case, as the author of RFC 2212, I'm in favor of this RFC, since this RFC is a step closer to the goal of RFC 2212 (and exceeds it on the issue of function pointers). Thank you!

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Apr 11, 2018

@parched

Am I right in assuming that no unsafe block is required here because currently we don't allow disabling features and have no negative features?

Yes, thanks for bringing this up. That example has to be written as:

fn foo() { }
#[target_feature(enable = "x")] fn bar() {
    foo(); // OK
}

If we were to allow "negative" features, then:

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

I'll add how this could be extended to negative features to the unresolved questions part of the RFC.


@hsivonen

Correct?

Yes, that's correct! However, you spotted the following bug which changes point 2.

this RFC requires the caller and the callee to have the exact same target_feature list

The guide-level explanation states:

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.

while the reference section states:

safe #[target_feature] functions can be called without an unsafe {} block only from functions with the exact same set of #[target_feature]s.

which obviously don't match. So which is it? First, this is a bug in the RFC, thank you for spotting this. The reference section is wrong and it should state what the guide-level explanation says. That is, the calle and the caller must have the same target features enabled, but the caller can have more target features enabled than the callee.


@hsivonen

I think this is quite unfortunate given how the SSE family is organized. I think it would be beneficial to define the hierarchy for the SSE family even if defining other hierarchies was deferred.

I think that now that std::arch is merged it would be enough to open a tracking-issue in rust-lang/rust that requests that #[target_feature(enable = "avx")] also enables sse2,3,4,4.2,.... That will need little motivation, but one will have to go through the specs collecting the information that such a hierarchy is guaranteed to work on all past, present, and future Intel/AMD CPUs. Depending on how that issue evolves a mini-FCP might be enough for that.

I prefer to not derail this RFC into a discussion about the hierarchy, but I'd gladly chime in into a tracking issue for that if somebody opens it.

@gnzlbg

This comment has been minimized.

Copy link
Owner Author

gnzlbg commented on text/0000-target-feature-1.1.md in b73fc95 Apr 11, 2018

@Centril I've added some wording about the interaction with an effect systems and some of the things we discussed on IRC. Sadly, my IRC logs are gone, so I might have missed a couple of things. Please triple-check this :)

This comment has been minimized.

Copy link

Centril replied Apr 12, 2018

It looks good to me =)

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Apr 11, 2018

@hsivonen

This RFC needs some information about the functions for the compiler to decide that meow is not allowed to call bar but bark is. What that information is called is a matter of terminology, but I don't think the two RFCs differ in substance on this point.

I was talking with @Centril on IRC the other day and I agree: this RFC does change the typing rules since programs that are now rejected would become accepted. For the cases introduced in this RFC, it might be enough to extend the checks of unsafe for expressions in librustc_mir. What this RFC does not do, and what RFC2212 did not do either, is make #[target_feature] a proper part of a function's type, so that one could, for example, let x: #[target_feature(...)] fn() -> () = foo(); and make the error a "type mismatch" error.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Nov 26, 2019

@joshtriplett I don't think that concern is blocking us from reviewing the design as I've provided my review of the design concurrently with the other concern.

I agree that we previously haven't gated approving RFCs on having people interested in the implementation, but I think that has been a problem and this is as good a time as any to start. It has been a problem because it spreads a largely volunteer-based set of compiler contributors thin by increasing the number of unfinished things and things which are in flight. In my view, it is in line with the thinking in the 2019 roadmap to focus on sustainability and maturity. Moving costs up-front and narrowing the funnel through which proposals need to be handled by the compiler team does that in my view.

As for discussion on meetings, I've mentioned several times that I think we should start moving in this direction.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Nov 26, 2019

This is off-topic for this RFC, and I don't think it's reasonable for us to block RFCs on such criteria until we have consensus on such criteria.

For my part, I don't think prearranging a specific developer to implement a feature should be a requirement to get a change approved by the language team.

gnzlbg and others added 7 commits Nov 27, 2019
Reword
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
Reword
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Nov 27, 2019

I found some minor issues that I'd like to see addressed before merging:

I think I fixed most of these. Nobody has proposed a way to be parametric over #[target_feature], so I've sketched a design that's similar to const Trait and explained why that doesn't work in general. If there is a design that's obvious for somebody, please feel free to chime in even though that might be a bit off-topic for this RFC. The main difference is that while it is ok to implement a fn trait method using a const fn, it is not ok to do the same with #[target_feature] (it would be akin to taking a fn trait method and implement it using an unsafe fn).

I also have a question around who is going to implement this...

I could implement this, but my availability is very low lately.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Jan 2, 2020

@rfcbot resolve review-comments

@Centril Centril removed the I-nominated label Jan 9, 2020
@Aaron1011

This comment has been minimized.

Copy link

Aaron1011 commented Jan 20, 2020

What's the status of this RFC?

@Lokathor

This comment has been minimized.

Copy link

Lokathor commented Jan 20, 2020

still stuck on concern who-is-going-to-implement?

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 22, 2020

In an effort to get this unstuck:

@gnzlbg I understand that you have limited availability. Rather than asking you to commit to implementing this, could you commit to answering questions for whoever does? (Ensuring that the RFC's "institutional memory" is maintained after being accepted.) I am hoping that might suffice, here, and might be an easier ask for you.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

gnzlbg commented Jan 24, 2020

Yes, I am available to mentor the implementation work if someone wants to go ahead and start it (just ping me in Zulip). I might become more available in February/March and might be able to do this myself but I cannot commit to that right now.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Jan 24, 2020

Alright. Let's hope someone does show up to ask you some of those questions. :)

@rfcbot resolve who-is-going-to-implement?

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 24, 2020

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

* 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

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jan 30, 2020

Member

Related RFC: #2585

But the proposal here still makes sense regardless of that.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 3, 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.

@nikomatsakis nikomatsakis mentioned this pull request Feb 12, 2020
0 of 3 tasks complete
nikomatsakis added a commit that referenced this pull request Feb 12, 2020
@nikomatsakis nikomatsakis merged commit 71b9069 into rust-lang:master Feb 12, 2020
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 12, 2020

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

Tracking issue: rust-lang/rust#69098

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.