-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add align_offset
intrinsic and [T]::align_to
function
#2043
Conversation
Under "Motivation":
I assume it does something like go byte-by-byte up until the alignment of a But later, under "How We Teach This":
...so this intrinsic could not be relied on for things like the UTF-8 validation mentioned in the motivation section? |
I'm not certain that this is the right public API. For example, I was looking at the mem::swap loop recently, and wondering whether it'd be worth aligning one of the three copies. But the way I'd default to writing that would be to mod by alignment to memcpy the prefix first, not a loop And a minor thing: the title of this makes me think, "intrinsics are internal implementation details; do they even need RFCs?". I'd like the "this is proposed for stability in core&std" to be more visible, such as at least in the summary (not just a line at the bottom of the design). |
Okay so I find it kinda funny that MIRI is not expected to support checking
alignment the old way (producing the dummy integer representations of
integers). Sure we'll add the intrinsic but what about
1. Code that checks for alignment that's not natural to the type? (I do not
remember there being an argument for the size of alignment and can't check
ATM);
2. Something like corrode converted code/code written by a C programmer who
would do the test the old way;
Then?
Furthermore it seems to me that MIRI is in a pecular spot where it has to
just support unaligned access for all types. So that even if it's (host's)
alignment requirements are different from the target requirements it has to
just work anyway.
…On Jun 28, 2017 05:00, "Screwtapello" ***@***.***> wrote:
Under "Motivation":
The standard library (and most likely many crates) [...] check whether a
pointer is aligned in order to perform optimizations like reading multiple
bytes at once. Not only is this code which is easy to get wrong, and which
is hard to read (and thus increasing the chance of future breakage) but it
also makes it impossible for miri to evaluate such statements. This means
that miri cannot do utf8-checking, since that code contains such
optimizations.
I assume it does something like go byte-by-byte up until the alignment of
a u64, then the SIMD accelleration kicks in?
But later, under "How We Teach This":
This means that while !is_aligned(ptr) { ... } should be considered a
potential infinite loop.
...so this intrinsic could not be relied on for things like the UTF-8
validation mentioned in the motivation section?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2043 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AApc0piiCPioXnqDNLBizijqrN__IMfDks5sIbO2gaJpZM4OFPYw>
.
|
Also i remember using this function from bluss crates which looks like
this:
fn alignto<T>(&[u8]) -> (&[u8], &[T], &[u8])
Perhaps we could just add that to the standard library and make Miri
somehow recognise it?
…On Jun 28, 2017 7:56 AM, "Simonas Kazlauskas" ***@***.***> wrote:
Okay so I find it kinda funny that MIRI is not expected to support
checking alignment the old way (producing the dummy integer representations
of integers). Sure we'll add the intrinsic but what about
1. Code that checks for alignment that's not natural to the type? (I do
not remember there being an argument for the size of alignment and can't
check ATM);
2. Something like corrode converted code/code written by a C programmer
who would do the test the old way;
Then?
Furthermore it seems to me that MIRI is in a pecular spot where it has to
just support unaligned access for all types. So that even if it's (host's)
alignment requirements are different from the target requirements it has to
just work anyway.
On Jun 28, 2017 05:00, "Screwtapello" ***@***.***> wrote:
> Under "Motivation":
>
> The standard library (and most likely many crates) [...] check whether a
> pointer is aligned in order to perform optimizations like reading multiple
> bytes at once. Not only is this code which is easy to get wrong, and which
> is hard to read (and thus increasing the chance of future breakage) but it
> also makes it impossible for miri to evaluate such statements. This means
> that miri cannot do utf8-checking, since that code contains such
> optimizations.
>
> I assume it does something like go byte-by-byte up until the alignment of
> a u64, then the SIMD accelleration kicks in?
>
> But later, under "How We Teach This":
>
> This means that while !is_aligned(ptr) { ... } should be considered a
> potential infinite loop.
>
> ...so this intrinsic could not be relied on for things like the UTF-8
> validation mentioned in the motivation section?
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <#2043 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AApc0piiCPioXnqDNLBizijqrN__IMfDks5sIbO2gaJpZM4OFPYw>
> .
>
|
You can always cast your pointer to a type supporting your desired alignment.
Well... right now we already don't support pointer arithmetic inside constants, so adding this feature will actually allow more code to work in const eval. You can always write code that won't be part of const eval.
This feature is needed for constant evaluation. CTFE cannot ever treat a pointer into any position of But even if we guarantee pointer alignment, we'd have the same issue for alignments above pointer alignment that we have right now for alignments bigger than
Well... the utf8-validation does not rely on the optimization to ever trigger. It would just keep on doing the unoptimized stuff.
That would work, as long as I'm allowed to implement it as fn alignto<T>(slice: &[u8]) -> (&[u8], &[T], &[u8]) {
(slice, &[], &[])
} Otherwise we have gained nothing. This API has the advantage over |
On Jun 28, 2017 10:14, "Oliver Schneider" <notifications@github.com> wrote:
1. Code that checks for alignment that's not natural to the type? (I do
not remember there being an argument for the size of alignment and can't
check ATM);
You can always cast your pointer to a type supporting your desired
alignment.
This is not true. There's no type with natural alignment of 4KB which you
might end up doing to align data to a page for example.
1. Something like corrode converted code/code written by a C programmer
who would do the test the old way;
Well... right now we already don't support pointer arithmetic inside
constants, so adding this feature will actually allow more code to work in
const eval. You can always write code that won't be part of const eval.
Okay so I find it kinda funny that MIRI is not expected to support checking
alignment the old way
This feature is needed for constant evaluation. CTFE cannot ever treat a
pointer into any position of [u8; N] as aligned to an alignment other than 1,
because it could either be &arr[0] which is aligned, or &arr[1] or... you
don't know before you run the program and the array is placed at some
arbitrary memory location. I mean we could decide that any const eval type
is aligned like usize, which is what C does (malloc guarantees that you get
at least the same alignment that the pointer type has). Rust doesn't
guarantee this, even if jemalloc does it (tested by allocating a few u8s on
the heap).
But even if we guarantee pointer alignment, we'd have the same issue for
alignments above pointer alignment that we have right now for alignments
bigger than 1.
...so this intrinsic could not be relied on for things like the UTF-8
validation mentioned in the motivation section?
Well... the utf8-validation does not rely on the optimization to ever
trigger. It would just keep on doing the unoptimized stuff.
fn alignto(&[u8]) -> (&[u8], &[T], &[u8])
the desired API is more like &[T] -> (&[T], &[U], &[T]),
That would work, as long as I'm allowed to implement it as
fn alignto<T>(slice: &[u8]) -> (&[u8], &[T], &[u8]) {
(slice, &[], &[])
}
Otherwise we have gained nothing. This API has the advantage over is_aligned
of being hard to use wrongly.
Yes that would be a valid implementation for the purposes of CTFE. That is
I believe that CTFE not handling alignment in any way is beneficial, even,
as it is a detail a number of layers below it. I do not exactly see an use
case for ctfe-time alignment handling either.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2043 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AApc0oOJiw-xqWH4sGa9-wXE9wLfkIy5ks5sIf07gaJpZM4OFPYw>
.
|
Ok, I agree that @nagisa the function splitting slices also doesn't support your 4KB alignment use case. But it would be addressed once |
is_aligned
intrinsicalignto
intrinsic and mem::alignto
function
@nagisa "Furthermore it seems to me that MIRI is in a pecular spot where it has to |
Thanks for the update, @oli-obk! Sorry for being slow to respond. A few thoughts:
|
Done. Plus added a note that we should extend the
Not just possible, but necessary. The optimizations in the stdlib can be very frickly (you can't always use iterators or loops). I added the most basic intrinsic that covers all use cases (basically the intrinsic is a more advanced form of the originally suggested
Just an oversight. Fixed.
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! Added a bunch more comments, but they're relatively minor.
text/0000-is-aligned-intrinsic.md
Outdated
Add a new intrinsic | ||
|
||
```rust | ||
fn alignto(ptr: *const (), align: usize) -> usize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a backend implementing this allowed to make any assumptions about the value of align?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an intrinsic, so I'd guess invalid alignments are UB. I'd assume a non-power of two alignment or an alignment bigger than 2^32
would be considered invalid.
If that is ok, I'll add it as documentation. That's the rules that the compiler internal code has around alignment.
The alternative would be to add a generic argument for the pointer, but that would be confusing, since you have to cast the pointer to the target type, and the pointee type would change behaviour. Pointee types are easy to get wrong in casts.
text/0000-is-aligned-intrinsic.md
Outdated
cannot be aligned. Since the caller needs to check whether the returned offset | ||
would be in-bounds of the allocation that the pointer points into, returning | ||
`usize::max_value()` will never be in-bounds of the allocation and therefor | ||
the caller cannot act upon the returned offset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of me thinks that this is more of a None
case, since it's fundamentally never actually needed to go more than align-1
bytes to find the desired alignment. But then it's a never-stable intrinsic, and the "needs to check the offset anyway" argument is a good one...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I considered this, but found that documenting that the result always needs to be checked much more realistic, especially since you might not want matches or map
+ closures in optimization critical code.
text/0000-is-aligned-intrinsic.md
Outdated
|
||
```rust | ||
fn alignto(ptr: *const (), align: usize) -> usize { | ||
align - (ptr as usize % align) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: are implementations expected to return align
for an already-aligned pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops... no.
text/0000-is-aligned-intrinsic.md
Outdated
under `core::mem` and `std::mem` that simplifies the common use case, returning | ||
the unaligned prefix, the aligned center part and the unaligned trailing elements. | ||
The function is unsafe because it essentially contains a `transmute<[U; N], T>` | ||
if one reads from the aligned slice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is pointer-based type punning, not transmute-based (memcpy) type punning.
text/0000-is-aligned-intrinsic.md
Outdated
|
||
Add an intrinsic (`fn alignto(ptr: *const (), align: usize) -> usize`) | ||
which returns the number of bytes that need to be skipped in order to correctly align the | ||
pointer `ptr` to `align`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshed: now that this doesn't return a pointer, maybe it should have a different name? Would help make it easier to talk about the two things, too, if the intrinsic and the eventually-stable methods had different names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I chose align_offset
for now, although byte_offset_to_aligned
might be clearer if wordier.
text/0000-is-aligned-intrinsic.md
Outdated
// due to `size_of::<T>() % size_of::<U>() == 0`, | ||
// the fact that `size_of::<U>() > align_of::<U>()`, | ||
// and the fact that `align_of::<T>() > align_of::<U>()` if `offset != 0` we know | ||
// that `offset % source_size == 0` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: technically we don't know this if the intrinsic returns usize::max()
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but in that case we get a value way bigger than slice.len()
and thus result in the case where head == slice
and mid
and tail
are empty. Which is the correct behaviour.
let head_count = offset / source_size;
let split_position = core::cmp::max(slice.len(), head_count);
let (head, tail) = slice.split_at(split_position);
But I just noticed that for zsts this function panics due to division by zero. Not sure what to do about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just a ZST check at the beginning that just returns (slice, &[], &[])
? It's not like aligned reads of nothing are better than unaligned reads of nothing 😆
text/0000-is-aligned-intrinsic.md
Outdated
on all current platforms. `alignto_mut` is expanded accordingly. | ||
|
||
Any backend may choose to always produce the following implementation, no matter | ||
the given arguments, since the intrinsic is purely an optimization aid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this text is left-over from when the slice version was the intrinsic? Seems like now it'll be something more like "if the backend compiles the intrinsic as return usize::max_value()
, this method will optimize down to the following"
text/0000-is-aligned-intrinsic.md
Outdated
|
||
```rust | ||
let ptr = v.as_ptr(); | ||
let align = ptr.offset(index).alignto(usize_bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's surprising to me that the text above says "Once can clearly see the three slice parts", but this code doesn't use the slice method. Also, consider linking to the full code -- I think, from the other issue, that it's https://github.com/rust-lang/rust/blob/f09576c4a41727a8d10bbfd8fd3fb2e10e1be3b3/src/libcore/str/mod.rs#L1433?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, yea, that's actually where I noticed the slice function is not sufficient
text/0000-is-aligned-intrinsic.md
Outdated
## Example 2 (slices) | ||
|
||
The `memchr` impl in the standard library explicitly uses the three phases of | ||
the `align_to` functions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshed/typo: align_to
here, alignto
in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be called align_to
in general? alignto feels unidiomatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept mistyping it as align_to
, too xD.
text/0000-is-aligned-intrinsic.md
Outdated
#[repr(align = 64)] | ||
struct TwoWord([usize; 2]); | ||
|
||
let (head, mid, tail) = std::mem::align_to<TwoWord>(text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the extra struct here necessary? I'd have thought that just align_to::<[usize;2], _>(text)
would be fine, since the original code only uses usize
-alignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I misread that code. It aligns to usize, but steps in two usize chunks.
Bikeshed: Upvote this comment if you want |
One way to ensure only powers of two are allowed in the align_to function is to take the power, not the alignment itself. |
@nagisa that would make it riddiculous to use... because you'd have to figure out the power of the result of |
Easy: 0usize.leading_zeros() - (std::mem::size_of::<T>()).leading_zeros() |
Retagging as libs help would be good on the method to expose in core, but doesn't really affect lang. You could also consider a T-compiler issue for just the intrinsic to start, and an RFC for the public API later. |
For the align parameter on the intrinsic, maybe it should just have the same rules as an align in Layout from Allocator? https://github.com/rust-lang/rfcs/blob/master/text/1398-kinds-of-allocators.md#layout-api Using the power is neat, since aligning can be done with |
Why would shifts be faster than |
That's a good question. There are a few unsafe inherent methods scattered around slice types, plenty on pointers since there's not much you can do with them that isn't unsafe. So I think there is actually a bit of precedent for inherent unsafe methods with unchecked indexing and slicing and whatnot. From an ergonomics perspective it seems like a win to me. |
Aspirationally, It'd be great to have a safe function that could do the |
@scottmcm I think the only way to get that to be safe would be to have a runtime check for matching sizes and some form of |
Hmm, typically unsafe variants would have a prefix/suffix and the safe variant wouldn't, but I'm tempted to ignore a potential safe variant as far as this RFC is concerned and we can figure it out in the future when all the pieces are available to make such a thing possible. Does that seem reasonable, or would you rather have some degree of future-proofing built in here? |
I don't think we can create a safe variant without some form for unsafe marker traits and a canonical transparent aligning wrapper type. I'd think that the actual alignment function would not take a generic argument anymore and would probably be on the wrapper type instead of on slices. But in any case, I definitely do not think that it should be named |
text/0000-is-aligned-intrinsic.md
Outdated
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
* produce a lint in case `sizeof<T>() % sizeof<U>() != 0` and in case the expansion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be another useful clippy
lint to have. Otherwise you'll just end up with something like (&[T], &[], &[])
which isn't useful, but also isn't the end of the world.
Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, 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. |
@KodrAus The |
@scottmcm Personally I think the fact that the function is unsafe should be enough to make users and future readers go read the scarily worded docs it will have and understand those edges better. But at the same time, I think it's definitely worthwhile to look at ways to make the function communicate its edges better, especially given it'll live on a very commonly used type. If everyone is on-board with communicating the fact that this is transmuting in the method name itself then we can do another round of thinking about the name. |
@scottmcm imo, the issue with |
Add align_offset intrinsic see rust-lang/rfcs#2043 for details and the plan towards stabilization (reexport in `core::mem` via various convenience functions) as per @scottmcm 's [comment](rust-lang/rfcs#2043 (comment)), this is just the intrinsic (which is obviously unstable).
🔔 This is now entering its final comment period, as per the review above. 🔔 |
alignto
intrinsic and mem::alignto
functionalign_offset
intrinsic and [T]::align_to
function
The final comment period is now complete. |
This RFC has been merged! Tracking issue. |
Rendered
cc @dwrensha @eddyb @joshlf @nikomatsakis
Background: rust-lang/miri#190 and rust-lang/rust#42561