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

[LLVM 4.0] Use 32-bits for alignment #39529

Merged
merged 2 commits into from
Feb 8, 2017

Conversation

dylanmckay
Copy link
Contributor

LLVM 4.0 changes this. This change is fine to make for LLVM 3.9 as we
won't have alignments greater than 2^32-1.

LLVM 4.0 changes this. This change is fine to make for LLVM 3.9 as we
won't have alignments greater than 2^32-1.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@dylanmckay dylanmckay mentioned this pull request Feb 4, 2017
23 tasks
@nagisa
Copy link
Member

nagisa commented Feb 4, 2017

All those casts make me uneasy. The functions called and variables used should become u32 in the first place IMO.

@hanna-kruppe
Copy link
Contributor

Yes, rustc should just follow LLVM's example and always compute and store alignments as 32 bit numbers.

@dylanmckay
Copy link
Contributor Author

There are two cases of casts that I've touched

  • Cases of bytes_to_bits(align). This function returns a u64. I can't make it return a u32 because the function is also uses for 64-bit sizes. One option is to make the function generic over numeric types.
  • Cases of variable as u32. All instances that I changed already were casting type_of::align_of from usize to u32, which seems unavoidable.

@hanna-kruppe
Copy link
Contributor

I think it makes sense to change bytes_to_bits to return the type of the argument, and then change all the computations of alignments (in bytes) that are fed to bytes_to_bits to use u32.

@dylanmckay
Copy link
Contributor Author

@rkruppe: I've removed all of the casts. It turns out that the llalign type was already u32 so the change was quite trivial.

Copy link
Contributor

@hanna-kruppe hanna-kruppe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! r=me, unless the people with bors rights would rather see @nagisa confirm.

@dylanmckay
Copy link
Contributor Author

Ping

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 8, 2017

📌 Commit c7bea76 has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 8, 2017
…crichton

[LLVM 4.0] Use 32-bits for alignment

LLVM 4.0 changes this. This change is fine to make for LLVM 3.9 as we
won't have alignments greater than 2^32-1.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Feb 8, 2017
…crichton

[LLVM 4.0] Use 32-bits for alignment

LLVM 4.0 changes this. This change is fine to make for LLVM 3.9 as we
won't have alignments greater than 2^32-1.
bors added a commit that referenced this pull request Feb 8, 2017
Rollup of 11 pull requests

- Successful merges: #39462, #39512, #39529, #39557, #39561, #39582, #39583, #39597, #39622, #39624, #39630
- Failed merges:
@bors bors merged commit c7bea76 into rust-lang:master Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants