Uh oh!
There was an error while loading. Please reload this page.
Add a flat address space to librustc_codegen_llvm - #62565
Closed
YakoYakoYokuYoku wants to merge 7 commits into
Closed
Add a flat address space to librustc_codegen_llvm#62565YakoYakoYokuYoku wants to merge 7 commits into
librustc_codegen_llvm#62565YakoYakoYokuYoku wants to merge 7 commits into
Conversation
rust-highfive
commented
Jul 10, 2019
Contributor
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
cramertj
commented
Jul 10, 2019
Member
r? @eddyb |
rust-highfive
commented
Jul 10, 2019
Contributor
The job 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 |
rust-highfive
commented
Jul 10, 2019
Contributor
The job 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 |
mati865
commented
Jul 11, 2019
Member
Changes to |
eddyb
commented
Jul 11, 2019
Contributor
There's a merge commit that shouldn't exist - you might need to rebase. |
eddyb
commented
Jul 11, 2019
Contributor
This seems to contain #51576, which was never shrunk enough for inclusion. |
YakoYakoYokuYokuforce-pushed
the
addrspaces
branch
from
July 11, 2019 12:34
67b23c3 to
c7b5154CompareAuthor
Last night I was panicking that everything wasn't working because I've crashed miserably with the While reviewing the code locally I've seen a lot of the code problems pointed out by @eddyb, like a few lines we could change. I've left them untouched 'cause I wanted to see if the PR itself could work with Edit: I'll add the tests in the |
Alexendoo
commented
Jul 24, 2019
Member
Ping from triage, any updates? @eddyb |
eddyb
commented
Jul 24, 2019
Contributor
@Alexendoo Sorry, this was incorrectly tagged. |
bors
commented
Jul 25, 2019
Collaborator
☔ The latest upstream changes (presumably #62961) made this pull request unmergeable. Please resolve the merge conflicts. |
Dylan-DPC-zz
commented
Jul 27, 2019
@eddyb they have pushed a new commit. You can review it now. |
bors
commented
Jul 28, 2019
Collaborator
☔ The latest upstream changes (presumably #63074) made this pull request unmergeable. Please resolve the merge conflicts. |
YakoYakoYokuYoku
commented
Jul 28, 2019
Author
@eddyb the conflicts are now solved, you can review this freely 👓 ✏️ |
edmilsonefs
commented
Aug 6, 2019
Hey! This is a ping from triage, we would like to know if you @eddyb could give us a few more minutes to update us from last changes made by @YakoYakoYokuYoku . Thanks. |
eddyb
reviewed
Aug 9, 2019
| assert!(!layout.is_unsized(), "tried to statically allocate unsized place"); | ||
| let tmp = bx.alloca(bx.cx().backend_type(layout), name, layout.align.abi); | ||
| Self::new_sized(tmp, layout, layout.align.abi) | ||
| } |
Contributor
There was a problem hiding this comment.
This is identical to alloca above though?
eddyb
reviewed
Aug 9, 2019
| } | ||
| }; | ||
| bx.ret(llval); | ||
| let llvalc = bx.flat_addr_cast(llval); |
Contributor
There was a problem hiding this comment.
IMO the only way this PR will be accepted without an RFC is if everything is cast to the flat address space as soon as possible (as opposed to when needed).
eddyb
reviewed
Aug 9, 2019
| } | ||
| fn type_ptr_to_mut(&self, ty: Self::Type) -> Self::Type { | ||
| self.type_as_ptr_to(ty, self.mutable_addr_space()) | ||
| } |
Contributor
There was a problem hiding this comment.
These methods shouldn't be added if they're unused.
eddyb
reviewed
Aug 9, 2019
| } | ||
| pub fn val_addr_space(v: &'ll Value) -> AddrSpaceIdx { | ||
| val_addr_space_opt(v).unwrap_or_default() |
Contributor
There was a problem hiding this comment.
Reflecting LLVM values should be avoided as much as possible. New uses of val_ty shouldn't be added.
wirelessringo
commented
Aug 16, 2019
Ping from triage. @YakoYakoYokuYoku any updates on this? Thanks @rustbot modify labels to +S-waiting-on-author, -S-waiting-on-review |
YakoYakoYokuYoku
commented
Aug 16, 2019
Author
Think It's better to prepare a proper RFC, closing this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To achieve compatibility with LLVM target machines which place allocas in another address space we need to the presence of a flat address space (an address space that is shared with other address spaces).
Also this PR updates #51576.