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.

Sign up for GitHub

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

Jump to bottom

Support memcpy/memmove with differing src/dst alignment #55633

Merged
merged 1 commit into from
Nov 9, 2018
Merged

Support memcpy/memmove with differing src/dst alignment #55633

merged 1 commit into from
Nov 9, 2018

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Nov 2, 2018

If LLVM 7 is used, generate memcpy/memmove with differing src/dst alignment. I've added new FFI functions to construct these through the builder API, which is more convenient than dealing with differing intrinsic signatures depending on the LLVM version.

Fixes #49740.

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Nov 2, 2018
@rust-highfive

This comment has been minimized.

Sign in to view
nagisa
nagisa reviewed Nov 3, 2018
View reviewed changes
src/librustc_codegen_llvm/base.rs
let volatile = C_bool(cx, flags.contains(MemFlags::VOLATILE));
bx.call(memcpy, &[dst_ptr, src_ptr, size, align, volatile], None);
let volatile = flags.contains(MemFlags::VOLATILE);
bx.memcpy(dst_ptr, dst_align.abi(), src_ptr, src_align.abi(), size, volatile);
}

pub fn memcpy_ty(
Copy link
Member

@nagisa nagisa Nov 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What is the benefit of allowing caller to specify different src layout and src alignment (contained within their respective layout) separately?

Seeing it is a convenience wrapper, it would make sense to simply take TyLayout for both dst and src and extract the necessary information from that.

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 don't think those alignments necessarily match. A type can be stored at a location with higher alignment than what the type layout alignment specifies. One example where this may happen is for field projections, where the alignment of the field is determined by the alignment of the containing structure and the field offset, not the layout alignment of the field type.

Copy link
Member

Choose a reason for hiding this comment

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

One example where this may happen is for field projections, where the alignment of the field is determined by the alignment of the containing structure and the field offset, not the layout alignment of the field type.

Fair. Although we don’t optimise this way currently, there were some discussion about doing so, so code might end up becoming what it looks like now anyway.

@nagisa
Copy link
Member

nagisa commented Nov 3, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2018

📌 Commit bea6b93673df3756961e2c6e3cf10df0a5aa26e8 has been approved by nagisa

@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 Nov 3, 2018
@bors
Copy link
Contributor

bors commented Nov 3, 2018

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 3, 2018
@estebank
Copy link
Contributor

estebank commented Nov 3, 2018

r? @alexcrichton

@estebank
Copy link
Contributor

estebank commented Nov 3, 2018

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned alexcrichton Nov 3, 2018
@nagisa
Copy link
Member

nagisa commented Nov 3, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Nov 3, 2018

📌 Commit a470395e58833f2536403e2c4ee12d7b49809739 has been approved by nagisa

@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 Nov 3, 2018
@nikic nikic mentioned this pull request Nov 4, 2018
Open
@bors
Copy link
Contributor

bors commented Nov 4, 2018

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 4, 2018
@nikic
Support memcpy/memmove with differing src/dst alignment
463ad90 
If LLVM 7 is used, generate memcpy/memmove with differing
src/dst alignment. I've added new FFI functions to construct
these through the builder API, which is more convenient than
dealing with differing intrinsic signatures depending on the
LLVM version.
@nikic
Copy link
Contributor Author

nikic commented Nov 4, 2018

Rebased

@nagisa
Copy link
Member

nagisa commented Nov 4, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2018

📌 Commit 463ad90 has been approved by nagisa

@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 Nov 4, 2018
@bors
Copy link
Contributor

bors commented Nov 8, 2018

⌛ Testing commit 463ad90 with merge 3cb9d37...

bors added a commit that referenced this pull request Nov 8, 2018
@bors
Auto merge of #55633 - nikic:memcpy-align, r=nagisa
3cb9d37 
Support memcpy/memmove with differing src/dst alignment

If LLVM 7 is used, generate memcpy/memmove with differing src/dst alignment. I've added new FFI functions to construct these through the builder API, which is more convenient than dealing with differing intrinsic signatures depending on the LLVM version.

Fixes #49740.
@bors
Copy link
Contributor

bors commented Nov 8, 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 Nov 8, 2018
@nikic
Copy link
Contributor Author

nikic commented Nov 8, 2018

Three hour timeout on one job, everything else passed. Probably spurious.

@alexcrichton
Copy link
Member

@bors: retry delegate+

@bors
Copy link
Contributor

bors commented Nov 8, 2018

✌️ @nikic can now approve this pull request

@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 Nov 8, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Nov 9, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#55633 - nikic:memcpy-align, r=nagisa
78cab36 
Support memcpy/memmove with differing src/dst alignment

If LLVM 7 is used, generate memcpy/memmove with differing src/dst alignment. I've added new FFI functions to construct these through the builder API, which is more convenient than dealing with differing intrinsic signatures depending on the LLVM version.

Fixes rust-lang#49740.
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Nov 9, 2018
Merged
bors added a commit that referenced this pull request Nov 9, 2018
@bors
Auto merge of #55803 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
36a50c2 
Rollup of 17 pull requests

Successful merges:

 - #55576 (Clarify error message for -C opt-level)
 - #55633 (Support memcpy/memmove with differing src/dst alignment)
 - #55638 (Fix ICE in msg_span_from_free_region on ReEmpty)
 - #55659 (rustc: Delete grouping logic from the musl target)
 - #55719 (Sidestep link error from rustfix'ed code by using a *defined* static.)
 - #55736 (Elide anon lifetimes in conflicting impl note)
 - #55739 (Consume optimization fuel from the MIR inliner)
 - #55742 (Avoid panic when matching function call)
 - #55753 (borrow_set: remove a helper function and a clone it uses)
 - #55755 (Improve creation of 3 IndexVecs)
 - #55758 ([regression - rust2018]: unused_mut lint false positives on nightly)
 - #55760 (Remove intermediate font specs)
 - #55761 (mir: remove a hacky recursive helper function)
 - #55774 (wasm32-unknown-emscripten expects the rust_eh_personality symbol)
 - #55777 (Use `Lit` rather than `P<Lit>` in `ast::ExprKind`.)
 - #55783 (Deprecate mpsc channel selection)
 - #55788 (rustc: Request ansi colors if stderr isn't a tty)

Failed merges:

r? @ghost
@bors bors merged commit 463ad90 into rust-lang:master Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

Assignees

@nagisa nagisa

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nikic @rust-highfive @nagisa @bors @estebank @alexcrichton