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

Add clarifying context to the most confusing pointer APIs #95851

Closed
wants to merge 1 commit into from

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Apr 9, 2022

No description provided.

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Apr 9, 2022
///
/// [`offset`]: #method.offset
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NB: I dropped the note that it was the "inverse" of offset because this is a genuinely confusing statement.

offset is an asymmetric binary operation, it's like talking about the "inverse" of xy. There are two answers! (yth root and logx)

@Gankra
Copy link
Contributor Author

Gankra commented Apr 9, 2022

cc @workingjubilee and @oli-obk, hope I'm not butchering the context here

@rust-log-analyzer

This comment has been minimized.

/// frustrating if using this pattern made it impossible for an operation to work in
/// const contexts. Unfortunately, observing any properties of a pointer's address
/// is a very dangerous and problematic operation in const contexts, because it's a huge
/// reproducibility issue (which has actual soundness implications with compilation units).
Copy link
Member

Choose a reason for hiding this comment

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

It's also, like, just impossible since we don't know the address at which allocations are placed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "it's bad for at least two simple reasons" is good enough, unless you think it's more important to focus on that detail over the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(my concern is that if you get too deep into the details people will get mired in insisting it can work/makes sense/can be adjusted to make sense, and I think "nondeterminism bad" is the simplest way to dismiss that all.)

Copy link
Member

Choose a reason for hiding this comment

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

I find "dangerous and problematic" a bit vague and not as convincing as "it's simply not possible". But, no strong opinion.

Copy link
Member

Choose a reason for hiding this comment

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

In my eyes nondeterminism is kind of a poor argument here, considering that in general this function is nondeterministic at runtime in the sense that your allocation/stack whatever could be aligned to different things from one execution to the next, right? I think many folks would probably also respond to this with "ok, sure, but I'm fine with it".

It also seems to me that if we really wanted to, we probably could do "better" (e.g., if we guaranteed that all linker-level allocations were 8-byte-aligned, and then propagated that information down, creating something like monomorphized constants by the "parent" alignment). It's just actually doing that is very hard and not particularly worthwhile (you'd end up with a lot of duplicate work I'd guess).

Is there a reason we need this paragraph, instead of moving directly to the next one, which makes no real argument for why this is the case, aside from justifying it being OK to not worry about it since "we're not doing const SIMD anyway"?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, we can constrain the non-determinism, and then some more things become possible. But others remain impossible (e.g. in your case, asking about any alignment >8).

The problem is not that allocation non-determinism makes the behavior of this function non-deterministic. The problem is that, as a const fn, this function is executed before allocation non-determinism has been resolved. Like, we'd literally have to predict the future to be able to always implement this as a const fn.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that makes sense. I think we could probably tackle all realistic programs in a similar way, but it's a pile of worms and a bunch of complexity. In any case, I'm not strictly opposed to the wording here, but it also feels like it's an unnecessary diversion -- what do you think about just skipping this middle paragraph, and moving directly to the lack of need for it, i.e., const not supporting SIMD etc anyway?

Copy link
Member

Choose a reason for hiding this comment

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

I personally would leave a short note saying that const-eval runs before addresses are picked, so the final alignment if unknoweable, and that's why this operation always "fails" during const.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we need this paragraph, instead of moving directly to the next one, which makes no real argument for why this is the case, aside from justifying it being OK to not worry about it since "we're not doing const SIMD anyway"?

puppy-dog eyes b-b-b-but

Copy link
Member

Choose a reason for hiding this comment

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

In my experience, SIMD programmers don't actually care about Miri's needs that much. It's not like they hate Miri, it just doesn't help them understand why they have to handle this. They want to instead hear something like,

"Yes, the promises are technically very weak, but this function will exercise a good faith effort to work as you would expect it to when your code actually runs on a hard processor. But if your code is executed on Miri's virtual machine, Miri is allowed to do really really weird things, like put all the data of your program in virtual registers instead of truly addressable virtual memory, and then give you pointers into the registers, and then alignment doesn't truly exist? It's... it's weird.

Just write code that is sound and this method will give it approximately the performance characteristics you would want at runtime. But if it is executed during CTFE, then the performance characteristics don't really matter, because the actual runtime cost becomes zero, and no matter how much SIMD acceleration you bring, you can't beat zero time."

@Gankra
Copy link
Contributor Author

Gankra commented Apr 9, 2022

Updated the description of offset_from for Ralf's review (good feedback!)

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2022
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 23, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2022
Comment on lines +1049 to +1053
/// Thankfully, because the precise SIMD pattern we're interested in *already* has a fallback
/// mode where the alignment completely fails, the compiler can just make this operation always
/// fail to align the pointer when doing const evaluation, and then everything is
/// always deterministic and reproducible (and the const evaluator is an interpreter anyway,
/// so you weren't actually going to get amazing SIMD speedups).
Copy link
Member

@workingjubilee workingjubilee Aug 2, 2022

Choose a reason for hiding this comment

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

More directly: given a slice with a random position in memory, we can't know the head of the slice is SIMD-aligned, and given a slice, we can have any number of values. Thus we must have a head and tail handling routine, because asserting on the size is not good enough (may be off-alignment) and asserting on the alignment is not good enough (may be an uneven number of elements, thus not trivially vectorized without tail-handling), and asserting on both just gives you a panic generator where a useful program should be, because you usually don't really have any way to pre-establish those guarantees in a useful manner based on the type information currently present in the system. And if you actually do, you have much bigger tricks to deploy on your code than this.

Thus, "you better believe you are going to be writing code to handle this correctly, Miri's semantics be damned."

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2022
@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2023
@bors
Copy link
Contributor

bors commented Sep 8, 2023

☔ The latest upstream changes (presumably #115672) made this pull request unmergeable. Please resolve the merge conflicts.

@Gankra Gankra closed this Sep 18, 2023
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.