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

Improve the documentation for std::hint::black_box. #62891

Open
wants to merge 1 commit into
base: master
from

Conversation

@vext01
Copy link
Contributor

commented Jul 23, 2019

The other day a colleague was reviewing some of my code which was using black_box to block constant propogation. There was a little confusion because the documentation kind of implies that black_box is only useful for dead code elimination, and only in benchmarking scenarios.

The docs currently say:

A function that is opaque to the optimizer, to allow benchmarks to pretend to use outputs to assist in avoiding dead-code elimination.

Here is our discussion, in which I show (using godbolt) that a black box can also block constant propagation:
softdevteam/yk#21 (comment)

This change makes the docstring for black_box a little more general, and while we are here, I've added an example (the same one from our discussion).

image

OK to go in?

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 23, 2019

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@cramertj

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

IIRC on the RFC we were specifically discussing not making these kinds of promises for this function. cc @RalfJung

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Indeed, see rust-lang/rfcs#2360 for the long and detailed discussion and rust-lang/rfcs#2360 (comment) for a kind of summary with links to some key posts.

If anything the documentation should be changed to be more like the text of that RFC, and less about being "opaque to the optimizer".

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

So something like:

an identity function that hints the compiler to be maximally pessimistic in terms of the assumptions about what black_box could do.

?

I guess I'm OK with it, even if it's a little vague.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Yes, something like that. Maybe better with an explicit statement that this is done on a "best effort" basis. Quoting from the RFC:

However, Rust implementations are encouraged to assume that
black_box can use x in any possible valid way that Rust code is allowed to
without introducing undefined behavior in the calling code. That is,
implementations are encouraged to be maximally pessimistic in terms of
optimizations.

This property makes black_box useful for writing code in which certain
optimizations are not desired, but too unreliable when disabling these
optimizations is required for correctness.

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

OK, so here's a preliminary proposal:

An identity function that hints the compiler to be maximally pessimistic about what black_box could do.

The compiler assumes that the argument to the black box could be read and/or mutated in a safe fashion (without undefined behaviour). In reality, the argument is neither read nor mutated.

This function is useful for (e.g.) preventing benchmarking code from being optimised away.

Questions:

  • Does black_box make any assumptions about side-effecting state other than its argument? IIRC, an inline assembler block is assumed to mutate anything, but I'm not sure if you are planning on exposing the implementation. From the RFC it sounds like the implementation may change.

  • If I understand correctly, a black box can also be used to anchor code relative to other black_box calls (preventing re-ordering). For example, if you sandwich code between two black boxes, then the code inside cannot be moved outside. Is that right? If so, should we mention this?

@rkruppe

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

For example, if you sandwich code between two black boxes, then the code inside cannot be moved outside. Is that right? If so, should we mention this?

We don't want to/know how to properly specify that any more than for other restrictions on optimizations, so it certainly wouldn't be guaranteed. Plus, it's not even true of the current implementation. As the simplest possible counter-example, in this snippet:

black_box(...); // doesn't mention x
let y = x + 1;
black_box(...);

even if the x + 1 can't be optimized in any other way, it is certainly possible for LLVM to move it (or duplicate it) before or after the two black_box calls. The black_box calls are considered to have side effects by LLVM, but the addition doesn't have side effects, and so there is no reason why it should have to remain in the same position relative to side effecting instructions.

(If you mean "sandwiched" in terms of data-flow, i.e. in the above example if the first black_box produces x and the second uses y, then practically speaking it's probably not possible to elide the addition in that place. But it's certainly still possible to e.g. duplicate the x + 1 and compute it again afte rthe second black_box, e.g. if there's another use of y.)

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

there is no reason why it should have to remain in the same position

OK, that wasn't my understanding. I thought that an inline asm block must be assumed to possibly modify x, or any variable for that matter, and thus it's position must not change.

Let's just not mention it?

@RalfJung

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

I feel like you removed all the words that indicated that this is an entirely best-effort hint, and no guarantee. I propose something like:

An identity function that hints the compiler to be maximally pessimistic about what black_box could do.

The compiler is encouraged to assume that black_box can use x in any possible valid way that Rust code is allowed to without introducing undefined behavior in the calling code. This property makes black_box useful for writing code in which certain optimizations are not desired (such as preventing benchmarking code from being optimized away), but too unreliable when disabling these optimizations is required for correctness.

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Reads OK to me, but ultimately I think the Rust devs should decide.

(In light of our conversation, I'm not certain I fully understand the intention of black_box)

@gnzlbg
Copy link
Contributor

left a comment

Might be better to just say that black_box is "A hint that might be treated as an unknown function by the optimizer."

And just leave it at that.

/// A function that is opaque to the optimizer, to allow benchmarks to
/// pretend to use outputs to assist in avoiding dead-code
/// elimination.
/// An identity function whose argument is opaque to the optimizer. This is useful for (e.g.)

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Jul 24, 2019

Contributor

I believe this is incorrect, the argument isn't opaque to the optimizer, and optimizations on the argument are possible, e.g., black_box(2 * 5) is currently optimized to black_box(10).

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Jul 24, 2019

Contributor

Also, "is opaque" is also incorrect - this function is a hint, there are no guarantees that it will do anything at all, and in some platforms, it does more than in others (e.g. using volatile reads vs inline assembly).

/// pretend to use outputs to assist in avoiding dead-code
/// elimination.
/// An identity function whose argument is opaque to the optimizer. This is useful for (e.g.)
/// benchmarking, where you want to avoid things like dead-code elimination and constant

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Jul 24, 2019

Contributor

This should probably be part of the body of the documentation and not of the "brief" string.

I would leave this as "This is useful for, e.g., benchmarking, to prevent benchmarks from being optimized away". There are no guarantees whatsoever about which optimizations this intends to inhibit.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Reads OK to me, but ultimately I think the Rust devs should decide.

I think that what @RalfJung proposes here (#62891 (comment)) is the best we can do, and from the RFC there is consensus that these are the semantics that the language team wants.

The only open issue in the RFC there is the one raised by @cramertj about giving this intrinsic a clearer name like bench_black_box, but this PR is not the place to do that.

cc @Centril @eddyb thoughts ?

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

cc @Centril @eddyb thoughts ?

Other than what has been said already, I think we should point out various scenarios people would think to use black_box for and how they cannot be used for them. (i.e. we should provide examples of "correctness" purposes, e.g. making UB go away and for security applications).

I think we should really emphasize the bit about "identity function" being the only guarantee a bit more.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Other than what has been said already, I think we should point out various scenarios people would think to use black_box for and how they cannot be used for them. (i.e. we should provide examples of "correctness" purposes, e.g. making UB go away and for security applications).

I think we should do this as well, but not necessarily in this PR. I'd leave it up to @vext01 to decide if they feel like doing that. If they don't, we should definitely open an issue to track doing that.

I think we should really emphasize the bit about "identity function" being the only guarantee a bit more.

We could add a sentence after the last bit that says "but too unreliable when disabling these optimizations is required for correctness.", e.g., "The only guaranteed semantics of black_box is that it is the identity function and Rust is allowed to optimize it as such, for example, removing it completely replacing black_box(x) with just x.".

That should make it clear that replacing black_box(x) with x is an allowed optimization and that users cannot write code that relies on it not happening for correctness.

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I think we should do this as well, but not necessarily in this PR.

Yeah entirely fair. At any point before stabilization should do from my POV. :)

@edmilsonefs

This comment has been minimized.

Copy link

commented Jul 30, 2019

Hey! This is a ping from triage, we would like to know if you @eddyb could give us a few minutes to share your thoughts on it.

@vext01 There is anything you would like to say/share based on the last responses you got so far?

Thanks.

@vext01

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@vext01 There is anything you would like to say/share based on the last responses you got so far?

I don't mind making the change if we can come to a consensus.

I'm not going to suggest anything myself, as I'm not sure I fully understand the intention of black_box. I had thought of it in terms of its implementation (opaque inline asm), but I think the Rust devs may have something else in mind... I got lost a while back I'm afraid.

@hdhoang

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Second reviewer ping from triage, pinging reviewer from T-compiler @varkor

@varkor

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

I agree that @RalfJung's suggestion is the most accurate description, possibly with @gnzlbg's addition. I'd be happy with an updated comment along those lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.