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

Implement simd_as for pointers #98441

Merged
merged 3 commits into from
Sep 17, 2022
Merged

Implement simd_as for pointers #98441

merged 3 commits into from
Sep 17, 2022

Conversation

calebzulawski
Copy link
Member

Expands simd_as (and simd_cast) to handle pointer-to-pointer, pointer-to-integer, and integer-to-pointer conversions.

cc @programmerjake @thomcc

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 24, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2022
Copy link
Member

@programmerjake programmerjake left a comment

Choose a reason for hiding this comment

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

lgtm

@workingjubilee
Copy link
Member

workingjubilee commented Jul 1, 2022

Hm. Unless I am misunderstanding which part of the interface you are trying to model here, this behavior should probably be a separate intrinsic rather than be behind simd_as or simd_cast, because the intention is that the pointer to integer cast will also become a separate intrinsic for Rust scalar code (to reflect its current separation in the existing Rust pointer API of addr and with_addr, et cetera... those will eventually become backed by intrinsics, instead of using as).

@programmerjake
Copy link
Member

programmerjake commented Jul 1, 2022

Hm. Unless I am misunderstanding which part of the interface you are trying to model here, this behavior should probably be a separate intrinsic rather than be behind simd_as or simd_cast, because the intention is that the pointer to integer cast will also become a separate intrinsic for Rust scalar code (to reflect its current separation in the existing Rust pointer API of addr and with_addr, et cetera... those will eventually become backed by intrinsics, instead of using as).

imho we need a simd equivalent to as as is currently used by scalar code for int <-> ptr casts, since that's what users currently expect. imho the strict_provenance things would be their own intrinsics, but wouldn't replace as since that's used for things like expose_addr and from_exposed_addr.

@programmerjake

This comment was marked as duplicate.

@calebzulawski
Copy link
Member Author

Yeah, there is no intention to make integer as pointer etc invalid. Since there currently is no new intrinsic, it's not really within the scope of portable SIMD to determine what that new intrinsic is. This change is necessary at a minimum to remove some transmutes from std::simd which are actually UB.

@workingjubilee
Copy link
Member

workingjubilee commented Jul 1, 2022

It's not clear to me we necessarily want the default cast to use the semantics of as, however (assuming we want a "default cast" for pointers at all), because as will actually be the cast that invalidates more optimizations.

cc @RalfJung

@RalfJung
Copy link
Member

RalfJung commented Jul 1, 2022

there is no intention to make integer as pointer etc invalid

Not sure what you mean by "invalid". At least some people intent to basically deprecate as because it has so many problems. For things like i8 as i32, the intended replacement is From/Into; for ptr as usize, the replacement is expose_addr, for usize as ptr, it is from_exposed_addr.

There is also ptr.addr() and ptr::invalid(int); those could in theory have SIMD versions as well.

Given all the problems with as in scalar code, I am not sure if it is the best idea to copy it for SIMD. And given all the subtleties around pointer/integer casts, I think a more explicit API such as expose_addr/from_exposed_addr is much preferable over the extremely implicit as.

@calebzulawski
Copy link
Member Author

I suppose by "intention" I meant that it's obviously a commonly used feature and requires an edition change, and future compilers will still need it to support old editions. Maybe in a (fairly far away) future edition of rust there will be no as, but I don't think it makes sense to wait until then for a perfect implementation when std::simd has UB today, which can be fixed.

Without as, how do I cast between pointer types?

@RalfJung
Copy link
Member

RalfJung commented Jul 2, 2022

Without as, how do I cast between pointer types?

https://doc.rust-lang.org/nightly/std/primitive.pointer.html#method.cast
This is safer since it cannot accidentally also change mutability.

We still need a method to explicitly change mutability. I didn't say we are there yet.

And indeed as casts will not be removed, but they might become discouraged and maybe even deprecated.


Anyway, I am mostly concerned about casts between pointers and integers. Those are very different from the other casts, and that's why we now have dedicated methods with very detailed documentation for ptr2int casts and int2ptr casts.

@calebzulawski
Copy link
Member Author

It's worth noting that I intend on implementing (and have already partially) the strict provenance API for std::simd--I just can't add some of these functions without a cast intrinsic.

@RalfJung
Copy link
Member

RalfJung commented Jul 2, 2022

Right, this is just the intrinsic, nothing user-facing.

However, I don't think it is a good idea to lump together expose_addr and ptr::from_exposed_addr with the other 'as'-casts. We are already having trouble with that in MIR; some optimizations that are valid for other casts are not valid for these two (or at least, not valid for expose_addr). That's why they are now a separate CastKind.

So IMO it would also be better to make their SIMD versions separate intrinsics.

@RalfJung
Copy link
Member

RalfJung commented Jul 2, 2022

Or should the semantics of these be that of addr and ptr::invalid? Those are not available as as-casts in scalar Rust. They have to be written via transmute (or by calling the unstable Strict Provenance methods).

@calebzulawski
Copy link
Member Author

I really just need the exposing casts, I've already implemented the others via transmute. Are there equivalent scalar pointer cast intrinsics I can reference? I implemented it with the as name because the comments in std seemed to indicate that there are no such intrinsics yet.

@RalfJung
Copy link
Member

RalfJung commented Jul 2, 2022

They are not exposed as intrinsics, they are (for historic reasons) exposed via as. However, they are a separate CastKind, and several MIR passes (and Miri) need to treat them different from all the other as casts.

IMO they should be separate Rvalue variants in MIR, but we haven't done that refactoring yet. No reason for SIMD to repeat the same mistakes, though. (I would not expect an intrinsic, for the same reason that regular scalar addition is also not an intrinsic.)

@calebzulawski
Copy link
Member Author

This is well above my head. Everything in portable SIMD is done via intrinsics, so I'm not really sure what the alternative is

@RalfJung
Copy link
Member

RalfJung commented Jul 2, 2022

I think what you should do is to have a dedicated intrinsic for "SIMD expose_addr", and another dedicated intrinsic for "SIMD from_exposed_addr", and have simd_as not support casting between pointers and integers.

That very roughly corresponds to how these operations are/should also be represented differently in MIR.

@michaelwoerister
Copy link
Member

r? rust-lang/libs (or is this even something for the lang team to discuss?)

@calebzulawski
Copy link
Member Author

I've changed this PR to add 3 new pointer intrinsics, rather than overload simd_as.

Copy link
Member

@programmerjake programmerjake left a comment

Choose a reason for hiding this comment

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

lgtm

@thomcc
Copy link
Member

thomcc commented Aug 27, 2022

@michaelwoerister Hmm, why did you request review from libs? This is all compiler code.

@programmerjake
Copy link
Member

programmerjake commented Aug 27, 2022

@michaelwoerister Hmm, why did you request review from libs? This is all compiler code.

i'd guess because we're expanding intrinsics' API so libs might have an opinion on their API.

@RalfJung
Copy link
Member

intrinsics are lang territory though. Exposing intrinsics is libs territory, but that's not what this PR does.

@thomcc
Copy link
Member

thomcc commented Aug 28, 2022

I can r+ this on behalf of libs, but not the lang/compiler parts.

@RalfJung
Copy link
Member

None of these simd intrinsics are exposed on stable, so I don't think lang has been involved much so far. I expect that will be all discussed together when (some subset of) std::simd moves towards stabilization.

@RalfJung
Copy link
Member

r? compiler

@rust-highfive rust-highfive assigned wesleywiser and unassigned kennytm Aug 28, 2022
@wesleywiser
Copy link
Member

r? compiler

@rust-highfive rust-highfive assigned oli-obk and unassigned wesleywiser Sep 9, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Sep 16, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 16, 2022

📌 Commit 3f2ce06 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 16, 2022
Implement simd_as for pointers

Expands `simd_as` (and `simd_cast`) to handle pointer-to-pointer, pointer-to-integer, and integer-to-pointer conversions.

cc `@programmerjake` `@thomcc`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#93628 (Stabilize `let else`)
 - rust-lang#98441 (Implement simd_as for pointers)
 - rust-lang#101790 (Do not suggest a placeholder to const and static without a type)
 - rust-lang#101807 (Disallow defaults on type GATs)
 - rust-lang#101915 (doc: fix redirected link in `/index.html`)
 - rust-lang#101931 (doc: Fix a typo in `Rc::make_mut` docstring)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cbd561d into rust-lang:master Sep 17, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 17, 2022
@calebzulawski
Copy link
Member Author

Thanks all!

@calebzulawski calebzulawski deleted the simd_as branch September 17, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.