Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js #59336

gnzlbg · 2019-03-21T08:24:10Z

This changes removes a cyclic dependency between the "test" and "libtest"
crates, where "libtest" depends on "test" for "black_box", but "test" depends on
"libtest" for everything else.

I've chosen the "hint" module because there seems to be enough consensus in the
discussion of RFC2360 that this module is where such an intrinsic would belong,
but this PR does not implement that RFC! If that RFC ever gets merged, the API, docs,
etc. of this API will need to change. This PR just move the implementation of the
already existing API.

For backwards compatibility reasons I've chosen to also keep the "test" feature
gate for these instead of adding a new feature gate. If we change the feature
gate, we'll potentially all benchmarks, and while that's something that we could
do, it seems unnecessary to do that now - if RFC2360 gets merged, we'll need to
do that anyways. Backwards compatibility is also why we continue to re-export
"black_box" from the "test" crate.

This PR also fixes black_box on the wasm32 target, which now supports inline assembly, and uses volatile loads on the asm.js target.

r? @Amanieu (cc @rust-lang/libs)

SimonSapin · 2019-03-21T12:37:12Z

This removes #[inline(never)] which seems significant. Is that deliberate?

Can you say more about what are what you call the "test" and "libtest" crate? In my mind these names refer to the same things. (Like we sometimes say std or libstd.)

gnzlbg · 2019-03-21T12:41:14Z

@SimonSapin note that only the implementation for some targets was #[inline(never)] before (the dummy implementation - the one using asm! does not need it). That's preserved here AFAICT.

gnzlbg · 2019-03-21T12:41:43Z

The test crate is rust-lang/rust/src/libtest, the libtest crate is rust-lang/libtest.

SimonSapin · 2019-03-21T12:45:53Z

Is there discussion you could link to (an rfc?) about why libtest exists and how it interacts with the other crate?

gnzlbg · 2019-03-21T12:50:07Z

The only RFC is custom test frameworks. There is an internals thread about the two crates: https://internals.rust-lang.org/t/a-path-forward-towards-re-usable-libtest-functionality-custom-test-frameworks-and-a-stable-bench-macro/9139

src/libcore/hint.rs

bors · 2019-03-23T00:47:18Z

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

This changes removes a cyclic dependency between the "test" and "libtest" crates, where "libtest" depends on "test" for "black_box", but "test" depends on "libtest" for everything else. I've chosen the "hint" module because there seems to be enough consensus in the discussion of RFC2360 that this module is where such an intrinsic would belong, but this PR does not implement that RFC! (note: if that RFC ever gets merged, the API, docs, etc. of this API will need to change). For backwards compatibility reasons I've chosen to also keep the "test" feature gate for these instead of adding a new feature gate. If we change the feature gate, we'll potentially all benchmarks, and while that's something that we could do, it seems unnecessary to do that now - if RFC2360 gets merged, we'll need to do that anyways.

…sembly

gnzlbg · 2019-03-25T12:55:37Z

Updated and rebased.

alexcrichton · 2019-03-25T14:17:37Z

@bors: r+

bors · 2019-03-25T14:17:38Z

📌 Commit cfa76c4 has been approved by alexcrichton

gnzlbg · 2019-03-25T17:41:18Z

@alexcrichton LLVM now supports inline assembly for wasm32, so I've moved the implementation of wasm32 to also use inline assembly here, and changed the PR name and description to reflect the changes.

This means that only the asm.js targets do something different here. This PR changes that to use a volatile read, which, as opposed to using #[inline(never)], at least does not get optimized away, but as @SimonSapin argues, this might be more expensive than necessary.

alexcrichton · 2019-03-25T18:16:05Z

@bors: r+

bors · 2019-03-25T18:16:06Z

📌 Commit 24db517 has been approved by alexcrichton

@Amanieu

Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js This changes removes a cyclic dependency between the "test" and "libtest" crates, where "libtest" depends on "test" for "black_box", but "test" depends on "libtest" for everything else. I've chosen the "hint" module because there seems to be enough consensus in the discussion of RFC2360 that this module is where such an intrinsic would belong, but this PR does not implement that RFC! If that RFC ever gets merged, the API, docs, etc. of this API will need to change. This PR just move the implementation of the already existing API. For backwards compatibility reasons I've chosen to also keep the "test" feature gate for these instead of adding a new feature gate. If we change the feature gate, we'll potentially all benchmarks, and while that's something that we could do, it seems unnecessary to do that now - if RFC2360 gets merged, we'll need to do that anyways. Backwards compatibility is also why we continue to re-export "black_box" from the "test" crate. This PR also fixes black_box on the wasm32 target, which now supports inline assembly, and uses volatile loads on the asm.js target. r? @Amanieu (cc @rust-lang/libs)

@Amanieu

Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js This changes removes a cyclic dependency between the "test" and "libtest" crates, where "libtest" depends on "test" for "black_box", but "test" depends on "libtest" for everything else. I've chosen the "hint" module because there seems to be enough consensus in the discussion of RFC2360 that this module is where such an intrinsic would belong, but this PR does not implement that RFC! If that RFC ever gets merged, the API, docs, etc. of this API will need to change. This PR just move the implementation of the already existing API. For backwards compatibility reasons I've chosen to also keep the "test" feature gate for these instead of adding a new feature gate. If we change the feature gate, we'll potentially all benchmarks, and while that's something that we could do, it seems unnecessary to do that now - if RFC2360 gets merged, we'll need to do that anyways. Backwards compatibility is also why we continue to re-export "black_box" from the "test" crate. This PR also fixes black_box on the wasm32 target, which now supports inline assembly, and uses volatile loads on the asm.js target. r? @Amanieu (cc @rust-lang/libs)

Centril · 2019-03-26T08:04:07Z

Failed in #59431 (comment), @bors r-

alexcrichton · 2019-03-26T14:31:04Z

src/libcore/hint.rs

+            crate::mem::forget(dummy);
+            ret
+        }
+    }


To simplify the #[cfg] here, perhaps this could be:

pub fn black_box<T>(dummy: T) -> { #[cfg(supports_asm)] { unsafe { asm!(...); } return dummy; } unsafe { let ret = read_volatile(...); ...; } }

Maybe with an #[allow(unreachable_code)] or something like that to pacify the linter?

The last commit adds cfg_if! to the internal_macros.rs and uses it here. I find this looks infinitely better with cfg_if! than with any of the other options, and I suppose we could access it from core::arch as well.

alexcrichton · 2019-03-26T15:55:56Z

@bors: r+

bors · 2019-03-26T15:55:57Z

📌 Commit 0c127e8 has been approved by alexcrichton

Centril · 2019-03-27T16:11:26Z

To unblock #59440, @bors p=1

bors · 2019-03-27T16:35:21Z

⌛ Testing commit 0c127e8 with merge 791242c16e3847a81bea38e2c80948930c5b4dbf...

bors · 2019-03-27T20:35:21Z

💥 Test timed out

Centril · 2019-03-27T20:37:40Z

@bors retry

bors · 2019-03-27T21:50:24Z

⌛ Testing commit 0c127e8 with merge 35d3949f6bb2e8be93c49a1c8be1aa1c421b2a4a...

bors · 2019-03-28T01:50:25Z

💥 Test timed out

kennytm · 2019-03-28T02:17:45Z

@bors retry

@Amanieu

Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js This changes removes a cyclic dependency between the "test" and "libtest" crates, where "libtest" depends on "test" for "black_box", but "test" depends on "libtest" for everything else. I've chosen the "hint" module because there seems to be enough consensus in the discussion of RFC2360 that this module is where such an intrinsic would belong, but this PR does not implement that RFC! If that RFC ever gets merged, the API, docs, etc. of this API will need to change. This PR just move the implementation of the already existing API. For backwards compatibility reasons I've chosen to also keep the "test" feature gate for these instead of adding a new feature gate. If we change the feature gate, we'll potentially all benchmarks, and while that's something that we could do, it seems unnecessary to do that now - if RFC2360 gets merged, we'll need to do that anyways. Backwards compatibility is also why we continue to re-export "black_box" from the "test" crate. This PR also fixes black_box on the wasm32 target, which now supports inline assembly, and uses volatile loads on the asm.js target. r? @Amanieu (cc @rust-lang/libs)

bors · 2019-03-28T05:44:11Z

⌛ Testing commit 0c127e8 with merge 6bfe4b7...

bors · 2019-03-28T08:56:27Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 6bfe4b7 to master...

rust-highfive assigned Amanieu Mar 21, 2019

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

alexcrichton reviewed Mar 21, 2019

View reviewed changes

src/libcore/hint.rs Outdated Show resolved Hide resolved

gnzlbg added 2 commits March 25, 2019 11:44

Remove dupplicated config

f244383

gnzlbg force-pushed the hint_black_box branch from f05930b to f244383 Compare March 25, 2019 10:44

black_box should inhibit optimizations on platforms without inline as…

cfa76c4

…sembly

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

gnzlbg changed the title ~~Moves test::black_box to core::hint~~ Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js Mar 25, 2019

black_box should use inline assembly on wasm32

24db517

gnzlbg force-pushed the hint_black_box branch from 962b1d2 to 24db517 Compare March 25, 2019 17:44

Centril mentioned this pull request Mar 26, 2019

Rollup of 12 pull requests #59428

Closed

Centril mentioned this pull request Mar 26, 2019

Rollup of 11 pull requests #59431

Closed

alexcrichton reviewed Mar 26, 2019

View reviewed changes

Life's too short not to use cfg_if

0c127e8

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 26, 2019

jonas-schievink mentioned this pull request Mar 26, 2019

Tracking issue for cfg_if #59442

Closed

gnzlbg mentioned this pull request Mar 26, 2019

[WIP] Update libtest to version 0.0.2 #59440

Closed

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 Mar 27, 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 Mar 27, 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 Mar 28, 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 Mar 28, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 28, 2019

bors merged commit 0c127e8 into rust-lang:master Mar 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js #59336

Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js #59336

gnzlbg commented Mar 21, 2019 •

edited

Loading

SimonSapin commented Mar 21, 2019

gnzlbg commented Mar 21, 2019

gnzlbg commented Mar 21, 2019

SimonSapin commented Mar 21, 2019

gnzlbg commented Mar 21, 2019

bors commented Mar 23, 2019

gnzlbg commented Mar 25, 2019

alexcrichton commented Mar 25, 2019

bors commented Mar 25, 2019

gnzlbg commented Mar 25, 2019 •

edited

Loading

alexcrichton commented Mar 25, 2019

bors commented Mar 25, 2019

Centril commented Mar 26, 2019

alexcrichton Mar 26, 2019

gnzlbg Mar 26, 2019

alexcrichton commented Mar 26, 2019

bors commented Mar 26, 2019

Centril commented Mar 27, 2019

bors commented Mar 27, 2019

bors commented Mar 27, 2019

Centril commented Mar 27, 2019

bors commented Mar 27, 2019

bors commented Mar 28, 2019

kennytm commented Mar 28, 2019

bors commented Mar 28, 2019

bors commented Mar 28, 2019

Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js #59336

Moves test::black_box to core::hint and fix black_box on wasm32 and asm.js #59336

Conversation

gnzlbg commented Mar 21, 2019 • edited Loading

SimonSapin commented Mar 21, 2019

gnzlbg commented Mar 21, 2019

gnzlbg commented Mar 21, 2019

SimonSapin commented Mar 21, 2019

gnzlbg commented Mar 21, 2019

bors commented Mar 23, 2019

gnzlbg commented Mar 25, 2019

alexcrichton commented Mar 25, 2019

bors commented Mar 25, 2019

gnzlbg commented Mar 25, 2019 • edited Loading

alexcrichton commented Mar 25, 2019

bors commented Mar 25, 2019

Centril commented Mar 26, 2019

alexcrichton Mar 26, 2019

Choose a reason for hiding this comment

gnzlbg Mar 26, 2019

Choose a reason for hiding this comment

alexcrichton commented Mar 26, 2019

bors commented Mar 26, 2019

Centril commented Mar 27, 2019

bors commented Mar 27, 2019

bors commented Mar 27, 2019

Centril commented Mar 27, 2019

bors commented Mar 27, 2019

bors commented Mar 28, 2019

kennytm commented Mar 28, 2019

bors commented Mar 28, 2019

bors commented Mar 28, 2019

gnzlbg commented Mar 21, 2019 •

edited

Loading

gnzlbg commented Mar 25, 2019 •

edited

Loading