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

vext01 · 2019-07-23T09:40:32Z

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:
ykjit/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).

OK to go in?

rust-highfive · 2019-07-23T09:40:36Z

r? @cramertj

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

cramertj · 2019-07-23T16:48:33Z

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

RalfJung · 2019-07-23T17:04:19Z

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 · 2019-07-24T10:16:37Z

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 · 2019-07-24T10:23:42Z

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 · 2019-07-24T10:40:58Z

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?

hanna-kruppe · 2019-07-24T11:07:22Z

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 · 2019-07-24T11:13:25Z

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 · 2019-07-24T13:40:00Z

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 · 2019-07-24T13:48:25Z

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

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.

gnzlbg · 2019-07-24T14:53:23Z

src/libcore/hint.rs

-/// 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.)


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).

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).

gnzlbg · 2019-07-24T14:55:16Z

src/libcore/hint.rs

-/// 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 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 · 2019-07-25T08:36:53Z

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 · 2019-07-25T08:51:16Z

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 · 2019-07-25T09:13:57Z

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 · 2019-07-25T09:16:34Z

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 · 2019-07-30T08:07:48Z

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 · 2019-07-30T21:56:49Z

@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 · 2019-08-09T01:42:32Z

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

varkor · 2019-08-11T22:33:51Z

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.

totsteps · 2019-08-20T21:27:20Z

Ping from triage, @vext01 as per varkor's comment can you please update the description as such.

Thanks

vext01 · 2019-08-21T10:53:45Z

I'm happy to make the edit with one clarification: I don't understand the last part of @RalfJung's suggestion.

If we elide the bracketed section for a moment, it reads:

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.

Seems to need a re-wording?

Centril · 2019-08-21T13:34:30Z

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.

Seems to need a re-wording?

Something like:

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

vext01 · 2019-08-21T13:53:39Z

However, this also makes black_box too unreliable when disabling these optimizations is required for correctness.

The revised comment makes sense, but if I understand correctly, you are saying "you can't rely on black_box to definitely do anything". If that's the case, I question why it exists it at all. (I don't see why an opaque asm block wouldn't always block Rust from seeing through it).

Regardless, if you are all happy with that wording I'll make the change.

Centril · 2019-08-21T13:57:13Z

The revised comment makes sense, but if I understand correctly, you are saying "you can't rely on black_box to definitely do anything". If that's the case, I question why it exists it at all. (I don't see why an opaque asm block wouldn't always block Rust from seeing through it).

You can definitely rely on black_box 1) moving the input, 2) returning the value back, 3) nothing else (it is only guaranteed to observably behave exactly like fn black_box<T>(x: T) -> T { x }.
(See the RFC for a discussion.)

vext01 · 2019-08-21T14:25:02Z

I've pushed the change, but honestly I'm not sure how much of an improvement it is. I think the wording will confuse the average Rust user.

If you folks think this makes sense, I can squash.

gnzlbg · 2019-08-21T15:08:19Z

If that's the case, I question why it exists it at all.

Because there are many use cases, like writing benchmarks, for which this guarantee is not necessary.

(I don't see why an opaque asm block wouldn't always block Rust from seeing through it).

We can't use asm blocks on all supported targets because not all of them allow inline assembly, but even for the targets in which we do, nothing prevents LLVM from starting doing optimization on inline assembly blocks, and optimizing the block we are using to nothing would be trivial.

vext01 · 2019-08-21T15:20:06Z

Thanks @gnzlbg -- that was the missing info.

In light of this, let me propose a revision to the wording. Watch this space.

src/libcore/hint.rs

Centril · 2019-08-21T15:01:20Z

src/libcore/hint.rs

+/// 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. However, this also makes `black_box` too unreliable when disabling these optimizations
+/// is required for correctness.


It would probably be good to include an example using #[bench] specifically.

I'd rather punt that to some follow up PR. If anything I'd require opening a tracking issue.

I agree. I'm running out of steam for black_box :)

RalfJung

LGTM. Let's see what CI says.

vext01 · 2019-08-24T17:51:07Z

CI says yes. Shall I squash?

RalfJung · 2019-08-24T17:54:30Z

Yes please do.

vext01 · 2019-08-24T19:17:24Z

Splat.

RalfJung · 2019-08-24T19:59:16Z

Thanks!

@gnzlbg @Centril are the last edits I suggested fine for you?

Centril · 2019-08-24T20:02:35Z

@RalfJung Good improvement; looks great. I think we can extend with examples of dos and don'ts in follow up PRs, but this is a good start.

gnzlbg · 2019-08-24T20:46:25Z

LGTM

RalfJung · 2019-08-25T08:14:37Z

@bors r=RalfJung,Centril,gnzlbg

bors · 2019-08-25T08:14:38Z

📌 Commit a4b3dbe has been approved by RalfJung,Centril,gnzlbg

bors · 2019-08-25T11:40:56Z

⌛ Testing commit a4b3dbe with merge b3145e9b935ace63f0e3adb416f14f60b8bf4db3...

bors · 2019-08-25T15:40:57Z

💥 Test timed out

RalfJung · 2019-08-25T16:04:06Z

@bors retry

bors · 2019-08-26T04:11:03Z

⌛ Testing commit a4b3dbe with merge e2b4165...

…ril,gnzlbg Improve the documentation for std::hint::black_box. 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: ykjit/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](https://user-images.githubusercontent.com/604955/61701322-ddf1e400-ad35-11e9-878c-b5b44a20770c.png) OK to go in?

bors · 2019-08-26T07:48:17Z

☀️ Test successful - checks-azure
Approved by: RalfJung,Centril,gnzlbg
Pushing e2b4165 to master...

vext01 · 2019-08-27T09:02:40Z

Thanks all.

I've raised an issue for the rustdoc bug:
#63944

rust-highfive assigned cramertj Jul 23, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 23, 2019

gnzlbg reviewed Jul 24, 2019

View reviewed changes

Centril reviewed Aug 21, 2019

View reviewed changes

RalfJung approved these changes Aug 24, 2019

View reviewed changes

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

a4b3dbe

vext01 force-pushed the improve-black-box-docs branch from abe9791 to a4b3dbe Compare August 24, 2019 19:16

gnzlbg approved these changes Aug 24, 2019

View reviewed changes

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 Aug 25, 2019

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 25, 2019

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 Aug 25, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 26, 2019

bors merged commit a4b3dbe into rust-lang:master Aug 26, 2019

vext01 deleted the improve-black-box-docs branch August 27, 2019 08:54

vext01 mentioned this pull request Aug 27, 2019

rustdoc: add a way to link relative to the root of the docs #63944

Open

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

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

Conversation

vext01 commented Jul 23, 2019 • edited Loading

rust-highfive commented Jul 23, 2019

cramertj commented Jul 23, 2019

RalfJung commented Jul 23, 2019

vext01 commented Jul 24, 2019

RalfJung commented Jul 24, 2019

vext01 commented Jul 24, 2019

hanna-kruppe commented Jul 24, 2019

vext01 commented Jul 24, 2019

RalfJung commented Jul 24, 2019 • edited Loading

vext01 commented Jul 24, 2019

gnzlbg left a comment

Choose a reason for hiding this comment

gnzlbg Jul 24, 2019

Choose a reason for hiding this comment

gnzlbg Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

gnzlbg Jul 24, 2019

Choose a reason for hiding this comment

gnzlbg commented Jul 25, 2019

Centril commented Jul 25, 2019

gnzlbg commented Jul 25, 2019 • edited Loading

Centril commented Jul 25, 2019

edmilsonefs commented Jul 30, 2019

vext01 commented Jul 30, 2019

hdhoang commented Aug 9, 2019

varkor commented Aug 11, 2019

totsteps commented Aug 20, 2019

vext01 commented Aug 21, 2019

Centril commented Aug 21, 2019

vext01 commented Aug 21, 2019

Centril commented Aug 21, 2019

vext01 commented Aug 21, 2019

gnzlbg commented Aug 21, 2019

vext01 commented Aug 21, 2019

Centril Aug 21, 2019

Choose a reason for hiding this comment

gnzlbg Aug 21, 2019

Choose a reason for hiding this comment

vext01 Aug 21, 2019

Choose a reason for hiding this comment

Centril Aug 21, 2019

Choose a reason for hiding this comment

RalfJung left a comment

Choose a reason for hiding this comment

vext01 commented Aug 24, 2019

RalfJung commented Aug 24, 2019 via email

vext01 commented Aug 24, 2019

RalfJung commented Aug 24, 2019

Centril commented Aug 24, 2019

gnzlbg commented Aug 24, 2019

RalfJung commented Aug 25, 2019

bors commented Aug 25, 2019

bors commented Aug 25, 2019

bors commented Aug 25, 2019

RalfJung commented Aug 25, 2019

bors commented Aug 26, 2019

bors commented Aug 26, 2019

vext01 commented Aug 27, 2019

vext01 commented Jul 23, 2019 •

edited

Loading

RalfJung commented Jul 24, 2019 •

edited

Loading

gnzlbg Jul 24, 2019 •

edited

Loading

gnzlbg commented Jul 25, 2019 •

edited

Loading