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

Refiling "#[repr(simd)] struct(isize, isize) not allowed" #55078

Closed
Centril opened this issue Oct 15, 2018 · 16 comments
Closed

Refiling "#[repr(simd)] struct(isize, isize) not allowed" #55078

Centril opened this issue Oct 15, 2018 · 16 comments
Labels
A-simd Area: SIMD (Single Instruction Multiple Data) C-feature-request Category: A feature request, i.e: not implemented / a PR. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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

@Centril
Copy link
Contributor

Centril commented Oct 15, 2018

Refiling rust-lang/rfcs#2513 here.

This probably does not require an RFC (according to me and @nrc).
However, we'd like @BurntSushi and @alexcrichton to sign off on this.

cc @rust-lang/compiler
cc @gnzlbg

Repro (playground: https://play.rust-lang.org/?gist=efa6e5caf752ccca651014ae5009feb8&version=nightly&mode=debug&edition=2015):

#![feature(repr_simd)] 

#[repr(simd)] struct A(isize, isize); // ERROR

#[cfg(target_pointer_width = "64")]
type isize_ = i64;

#[cfg(target_pointer_width = "32")]
type isize_ = i32;

#[repr(simd)] struct B(isize_, isize_); // Q: Is this ok?

produces

error[E0077]: SIMD vector element type should be machine type
 --> src/main.rs:3:15
  |
3 | #[repr(simd)] struct A(isize, isize);
  |               ^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

I am wondering why aren't isize and usize allowed ? I currently work around this to implement simd vectors of pointers, but maybe that's broken for reason I don't understand?

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Oct 15, 2018
@Centril
Copy link
Contributor Author

Centril commented Oct 15, 2018

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Oct 15, 2018

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

Concerns:

Once a majority of reviewers approve (and none object), 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 Oct 15, 2018
@Centril
Copy link
Contributor Author

Centril commented Oct 15, 2018

@Centril Centril added the A-simd Area: SIMD (Single Instruction Multiple Data) label Oct 15, 2018
@Centril
Copy link
Contributor Author

Centril commented Oct 15, 2018

@rfcbot concern signoff-from-alex-and-andrew

@scottmcm
Copy link
Member

This is just adding another type to a feature that isn't on the path to stabilization, right?

@rfcbot reviewed

@Centril
Copy link
Contributor Author

Centril commented Oct 15, 2018

@scottmcm yep :) (#27731 tracks repr(simd) in general)

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 15, 2018

While #[repr(simd)] struct A(isize, isize); currently errors, #[repr(simd)] struct A<T>(T, T); type B = A<isize>; currently accidentally works on nightly.

The packed_simd crate has used this "bug" to export vectors of pointers and vectors of usize/isize/msize for a while and uses them in some of the examples (e.g. the examples/aobench tiled.rs algorithm).

Lifting the restriction makes something that's already possible and useful a bit easier to do. Also, the current plan is to never stabilize repr(simd), but to expose it through a std API. On stable, some of these are already exposed via std::arch (e.g. __m128), but none of them interact with isize/usize - so this does not interact with stable types at all.

Something like RFC2366 with support for packed SIMD vectors of pointers would need to be added to the language for this to land on stable.

@alexcrichton
Copy link
Member

The #[repr(simd)] feature currently (and I can't forsee at least) have any path to stabilization. In that sense I would not personally recommend working on this feature as it's basically just a building block for std::arch, which we must expose to the language somehow.

I suspect, however, that fixing this is probably a few lines of a patch. While I wouldn't at all prioritize this I wouldn't stop it if someone were motivated.

@varkor
Copy link
Member

varkor commented Oct 15, 2018

I suspect, however, that fixing this is probably a few lines of a patch.

Namely, removing this line:

Int(ast::IntTy::Isize) | Uint(ast::UintTy::Usize) => false,

@Centril
Copy link
Contributor Author

Centril commented Nov 19, 2018

I'll interpret #55078 (comment) as "signoff" :)

@rfcbot resolve signoff-from-alex-and-andrew

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 19, 2018
@rfcbot
Copy link

rfcbot commented Nov 19, 2018

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 19, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 19, 2018

FWIW I actually had forgotten that I filled the RFC issue that resulted in this one being open.

I just wanted to know why did someone go through the trouble of preventing isize/usize in vector types. I couldn't find any reasons for this, the code has no comments, and we have been using them for a long time in packed_simd just fine. I just wanted to triple-check that we didn't miss anything.

I'd guess the answer is that no, we didn't miss anything.

Since this "bug" is easy to fix, maybe it should be E-Easy and mentoring available.

But I wasn't suggesting that we should fix this. It is trivial to work around (and fix), and it only impacts perma-unstable internal rustc APIs. Having an FCP about this feels extremely overkill.

@Centril Centril added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Nov 19, 2018
@eddyb
Copy link
Member

eddyb commented Nov 19, 2018

@gnzlbg It's possible the code used a categorization that dates back to when Rust had (or wanted to have?) bigints for int/uint and the types with numbers in them were "machine integers".

EDIT: nope, it was added in #6214, with no discussion in sight about int, uint and float.

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Nov 29, 2018
@rfcbot
Copy link

rfcbot commented Nov 29, 2018

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

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 29, 2018
@varkor varkor added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Nov 29, 2018
@varkor
Copy link
Member

varkor commented Nov 29, 2018

Mentoring instructions:

  1. Remove the line mentioned in Refiling "#[repr(simd)] struct(isize, isize) not allowed" #55078 (comment).
  2. Add a test like the mentioned in the issue post: Refiling "#[repr(simd)] struct(isize, isize) not allowed" #55078 (comment).

lambda added a commit to lambda/rust that referenced this issue Mar 15, 2019
As discussed in rust-lang#55078, there's no known reason for this restriction.

It's unlikely that repr(simd) will be stabilized in its current form, but
might as well remove some restrictions on it.

This removes the branch in `is_machine` which returns false for these types.
`is_machine` is only used for the repr(simd) type validation check.
Centril added a commit to Centril/rust that referenced this issue Mar 16, 2019
…-restriction, r=alexcrichton

Remove restriction on isize/usize in repr(simd)

As discussed in rust-lang#55078, there's no known reason for this restriction.

It's unlikely that repr(simd) will be stabilized in its current form, but
might as well remove some restrictions on it.

This removes the branch in `is_machine` which returns false for these types.
`is_machine` is only used for the repr(simd) type validation check.
kennytm added a commit to kennytm/rust that referenced this issue Mar 16, 2019
…-restriction, r=alexcrichton

Remove restriction on isize/usize in repr(simd)

As discussed in rust-lang#55078, there's no known reason for this restriction.

It's unlikely that repr(simd) will be stabilized in its current form, but
might as well remove some restrictions on it.

This removes the branch in `is_machine` which returns false for these types.
`is_machine` is only used for the repr(simd) type validation check.
@polybuildr
Copy link
Contributor

Looks like this was fixed in PR #59201 and merged by bors. Should this issue be closed now?

@Centril Centril closed this as completed Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-simd Area: SIMD (Single Instruction Multiple Data) C-feature-request Category: A feature request, i.e: not implemented / a PR. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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

No branches or pull requests

8 participants