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

Implement [T]::align_to #50319

Merged
merged 4 commits into from
May 19, 2018
Merged

Implement [T]::align_to #50319

merged 4 commits into from
May 19, 2018

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Apr 29, 2018

Note that this PR deviates from what is accepted by RFC slightly by making align_offset to return an offset in elements, rather than bytes. This is necessary to sanely support [T]::align_to and also simply makes more sense™. The caveat is that trying to align a pointer of ZST is now an equivalent to is_aligned check, rather than anything else (as no number of ZST elements will align a misaligned ZST pointer).

It also implements the align_to slightly differently than proposed in the RFC to properly handle cases where size of T and U aren’t co-prime.

Furthermore, a promise is made that the slice containing Us will be as large as possible (contrary to the RFC) – otherwise the function is quite useless.

The implementation uses quite a few underhanded tricks and takes advantage of the fact that alignment is a power-of-two quite heavily to optimise the machine code down to something that results in as few known-expensive instructions as possible. Currently calling ptr.align_offset with an unknown-at-compile-time align results in code that has just a single "expensive" modulo operation; the rest is "cheap" arithmetic and bitwise ops.

cc #44488 @oli-obk

As mentioned in the commit message for align_offset, many thanks go to Chris McDonald.

@rust-highfive
Copy link
Collaborator

r? @BurntSushi

(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 29, 2018
@@ -85,12 +86,12 @@ fn get_simple_intrinsic(cx: &CodegenCx, name: &str) -> Option<ValueRef> {
/// Remember to add all intrinsics here, in librustc_typeck/check/mod.rs,
/// and in libcore/intrinsics.rs; if you need access to any llvm intrinsics,
/// add them to librustc_trans/trans/context.rs
pub fn trans_intrinsic_call<'a, 'tcx>(bx: &Builder<'a, 'tcx>,
pub fn trans_intrinsic_call<'a, 'tcx>(bx: Builder<'a, 'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

Why is the signature of this function changed? From the diff it doesn't look like you really need the ownership.

Copy link
Member Author

Choose a reason for hiding this comment

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

Generating a call to the language item will terminate the block being passed in, so using that block further makes no sense. Passing ownership models this pretty well.

Copy link
Member

Choose a reason for hiding this comment

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

Is this just a refactoring or necessary for the new align_offset? If it's just refactoring I prefer this being separated into its own commit.

Copy link
Member Author

@nagisa nagisa May 2, 2018

Choose a reason for hiding this comment

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

Without the new align_offset this change is not necessary. This change is necessary for implementation of align_offset itself.

@nagisa
Copy link
Member Author

nagisa commented May 3, 2018

I’ll probably redo the PR slightly to not use an intrinsic at all – miri will still be able to detect calls to this by looking at langitem defid of the function.

@oli-obk
Copy link
Contributor

oli-obk commented May 3, 2018

I think we already do that for some u128 lowerings

@nagisa nagisa force-pushed the align_to branch 3 times, most recently from f27ddd1 to 9b7a579 Compare May 3, 2018 22:20
@nagisa
Copy link
Member Author

nagisa commented May 4, 2018

So, adapting the code was pretty easy, however it seems to have a hard hit on code quality in debug builds.

Most notably, constant propagation is not active, so the dead code is not removed, making what should essentially be p - (p % a) for sizeof::<T>() == 1 a significantly larger function (and includes all the dead code for the other cases). Optimised code is as good as one could hope, though.

I’m still going to go with this, because all the issues can be fixed with some const evaluation at a later date.

@scottmcm
Copy link
Member

scottmcm commented May 5, 2018

miri will still be able to detect calls to this by looking at langitem defid of the function.

I recall that this approach caused some problems with the i128 stuff, since it could get inlined and disappear. (It'd be nice, now that MIRI is merged, so go back and redo the lowering functions as const fn so they wouldn't need to be special at eval time, though last I tried intrinsics couldn't be marked const...)

@nagisa
Copy link
Member Author

nagisa commented May 5, 2018

const functions can’t do conditional code yet due to #49146.

@bors
Copy link
Contributor

bors commented May 6, 2018

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

@pietroalbini
Copy link
Member

Ping from triage @BurntSushi! This PR needs your review.

@nagisa
Copy link
Member Author

nagisa commented May 10, 2018

Perhaps somebody else from the @rust-lang/libs wants to volunteer to review this?

@pietroalbini pietroalbini added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 14, 2018
@pietroalbini
Copy link
Member

Ping from triage! This PR needs a review, can @BurntSushi or someone else from @rust-lang/libs review this?

@alexcrichton
Copy link
Member

This seems like it's a lot of unstable internal details that are unlikely to be stabilized soon so I'm not necessarily reviewing too too closely. @oli-obk does this overall change look ok to you? I think you were one of the original proponents for the intrinsic, right?

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2018

@oli-obk does this overall change look ok to you?

Yes, I already talked with @nagisa every now and then about it and have reviewed the current state last week. This is fine with me, though I can't grok the math, the tests look like it works out.

I think you were one of the original proponents for the intrinsic, right?

I created the rfc and initial impl. We still need this for miri to continue working.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 16, 2018

📌 Commit cdc5f33 has been approved by alexcrichton

@bors 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 May 16, 2018
@bors
Copy link
Contributor

bors commented May 17, 2018

⌛ Testing commit cdc5f334616211a3620a2a50239ec11c7b56afa1 with merge 5c5dc122a7cc2ed813958e12c595f67c4ea5c8cd...

@bors
Copy link
Contributor

bors commented May 17, 2018

💔 Test failed - status-appveyor

@bors 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 May 17, 2018
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 17, 2018
Keep only the language item. This removes some indirection and makes
codegen worse for debug builds, but simplifies code significantly, which
is a good tradeoff to make, in my opinion.

Besides, the codegen can be improved even further with some constant
evaluation improvements that we expect to happen in the future.
@nagisa
Copy link
Member Author

nagisa commented May 17, 2018

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented May 17, 2018

📌 Commit 59bb0fe has been approved by alexcrichton

@bors 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 May 17, 2018
@bors
Copy link
Contributor

bors commented May 18, 2018

⌛ Testing commit 59bb0fe with merge ccd0949aa5b6b143c3f16f7324c19868de26ad2b...

@bors
Copy link
Contributor

bors commented May 18, 2018

💔 Test failed - status-travis

@bors 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 May 18, 2018
@rust-highfive
Copy link
Collaborator

The job i686-gnu-nopt of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:03:02]    Compiling build_helper v0.1.0 (file:///checkout/src/build_helper)
[00:03:06]    Compiling std v0.0.0 (file:///checkout/src/libstd)
[00:03:11]    Compiling compiler_builtins v0.0.0 (file:///checkout/src/rustc/compiler_builtins_shim)
[00:03:11]    Compiling alloc_jemalloc v0.0.0 (file:///checkout/src/liballoc_jemalloc)
[00:03:23] warning: method is never used: `align_to_offsets`
[00:03:23]     --> libcore/slice/mod.rs:1701:5
[00:03:23]      |
[00:03:23] 1701 |     fn align_to_offsets<U>(&self) -> (usize, usize) {
[00:03:23]      |
[00:03:23]      = note: #[warn(dead_code)] on by default
[00:03:23] 
[00:03:23] 
[00:03:23] warning: function is never used: `gcd`
[00:03:23]     --> libcore/slice/mod.rs:1721:9
[00:03:23]      |
[00:03:23] 1721 |         fn gcd(a: usize, b: usize) -> usize {
[00:03:23] 
[00:03:31] [RUSTC-TIMING] core test:false 28.587
[00:03:33] [RUSTC-TIMING] compiler_builtins test:false 2.209
[00:03:33]    Compiling libc v0.0.0 (file:///checkout/src/rustc/libc_shim)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job i686-gnu-nopt of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@kennytm kennytm 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 May 18, 2018
@nagisa
Copy link
Member Author

nagisa commented May 18, 2018

I don’t get this failure. The log just… stops… as if somebody flicked the power switch in Travis’ data centre.

@kennytm
Copy link
Member

kennytm commented May 18, 2018

@bors retry

@bors 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 May 18, 2018
@bors
Copy link
Contributor

bors commented May 18, 2018

⌛ Testing commit 59bb0fe with merge 37a4091...

bors added a commit that referenced this pull request May 18, 2018
Implement [T]::align_to

Note that this PR deviates from what is accepted by RFC slightly by making `align_offset` to return an offset in elements, rather than bytes. This is necessary to sanely support `[T]::align_to` and also simply makes more sense™. The caveat is that trying to align a pointer of ZST is now an equivalent to `is_aligned` check, rather than anything else (as no number of ZST elements will align a misaligned ZST pointer).

It also implements the `align_to` slightly differently than proposed in the RFC to properly handle cases where size of T and U aren’t co-prime.

Furthermore, a promise is made that the slice containing `U`s will be as large as possible (contrary to the RFC) – otherwise the function is quite useless.

The implementation uses quite a few underhanded tricks and takes advantage of the fact that alignment is a power-of-two quite heavily to optimise the machine code down to something that results in as few known-expensive instructions as possible. Currently calling `ptr.align_offset` with an unknown-at-compile-time `align` results in code that has just a single "expensive" modulo operation; the rest is "cheap" arithmetic and bitwise ops.

cc #44488 @oli-obk

As mentioned in the commit message for align_offset, many thanks go to Chris McDonald.
@bors
Copy link
Contributor

bors commented May 19, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 37a4091 to master...

@bors bors merged commit 59bb0fe into rust-lang:master May 19, 2018
@nagisa
Copy link
Member Author

nagisa commented May 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants