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

Add Armv8-M Mainline targets #56000

Merged
merged 1 commit into from
Dec 7, 2018
Merged

Add Armv8-M Mainline targets #56000

merged 1 commit into from
Dec 7, 2018

Conversation

hug-dev
Copy link
Contributor

@hug-dev hug-dev commented Nov 16, 2018

This commit enables the Armv8-M Mainline architecture profile.
It adds two targets:

  • thumbv8m.main-none-eabi
  • thumbv8m.main-none-eabihf

The second one uses the Floating Point Unit for floating point
operations. It mainly targets the Cortex-M33 processor, which
can have the optional Floating Point Unit extension.

It follows #55041 which does it for Baseline. I will rebase this branch on top of it when it is merged to not create conflicts as we have some files in common. To make it work, it still relies on the Cargo change to be merged (accepting "." in target names, rust-lang/cargo#6255).

The goal would also be to add this target in the CI so that the core library is available for everybody. To do this, some changes will be needed to compile successfully the needed libraries:

  • cc-rs needs to be updated to allow compiling C code for Armv8-M architectures profiles. It is only a few lines to add here.
  • Some assembly files in builtins in compiler-rt were not assembling for Armv8-M Mainline. I sent changes upstream to that project to fix that. The Rust version of compiler-rt will have to be updated to contain that commit.

I tested it using the Musca-A Test Chip board but more intensively on the Armv8-M FVP (emulation platform). I am going to try to release my test code soon, once I tidy it up 👍

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2018
@jamesmunns
Copy link
Member

CC #55041 - which I think does the same

@jamesmunns
Copy link
Member

Ah sorry, now I've read the text, I missed the distinction between baseline and mainline :)

@zackmdavis
Copy link
Member

(I don't think I'm the most qualified reviewer for this one.)

r? @alexcrichton


pub fn target() -> TargetResult {
Ok(Target {
llvm_target: "thumbv8m.main-none-eabi".to_string(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed this one should be eabihf.

@alexcrichton
Copy link
Member

I think it's best to hold off on merging until rust-lang/cargo#6255 is sorted out (and #55041), but can get back to this after those!

@Dylan-DPC-zz Dylan-DPC-zz added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2018
@nagisa
Copy link
Member

nagisa commented Dec 2, 2018

Cargo PR has been merged.

@hug-dev
Copy link
Contributor Author

hug-dev commented Dec 3, 2018

I will rebase on top of #55041 when it is merged, including the change pointed by @parched

@bors
Copy link
Contributor

bors commented Dec 4, 2018

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

This commit enables the Armv8-M Mainline architecture profile.
It adds two targets:
  - thumbv8m.main-none-eabi
  - thumbv8m.main-none-eabihf

The second one uses the Floating Point Unit for floating point
operations. It mainly targets the Cortex-M33 processor, which
can have the optional Floating Point Unit extension.
@hug-dev
Copy link
Contributor Author

hug-dev commented Dec 6, 2018

@alexcrichton This one is now ready to be reviewed, if you have time

@alexcrichton
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Dec 6, 2018

📌 Commit 0f47c2a 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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Dec 6, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Dec 7, 2018
Add Armv8-M Mainline targets

This commit enables the Armv8-M Mainline architecture profile.
It adds two targets:
  - `thumbv8m.main-none-eabi`
  - `thumbv8m.main-none-eabihf`

The second one uses the Floating Point Unit for floating point
operations. It mainly targets the Cortex-M33 processor, which
can have the optional Floating Point Unit extension.

It follows rust-lang#55041 which does it for Baseline. I will rebase this branch on top of it when it is merged to not create conflicts as we have some files in common. To make it work, it still relies on the Cargo change to be merged (accepting "." in target names, rust-lang/cargo#6255).

The goal would also be to add this target in the CI so that the `core` library is available for everybody. To do this, some changes will be needed to compile successfully the needed libraries:

* `cc-rs` needs to be updated to allow compiling C code for Armv8-M architectures profiles. It is only a few lines to add [here](https://github.com/alexcrichton/cc-rs/blob/a76611ad9836fa8c44fa8220a1d2a96dd3b7d4b6/src/lib.rs#L1299).
* Some assembly files in `builtins` in `compiler-rt` were not assembling for Armv8-M Mainline. I sent changes [upstream](https://reviews.llvm.org/D51854) to that project to fix that. The Rust version of `compiler-rt` will have to be updated to contain [that commit](llvm-mirror/compiler-rt@a34cdf8).

I tested it using the [Musca-A Test Chip board](https://developer.arm.com/products/system-design/development-boards/iot-test-chips-and-boards/musca-a-test-chip-board) but more intensively on the [Armv8-M FVP](https://developer.arm.com/products/system-design/fixed-virtual-platforms) (emulation platform). I am going to try to release my test code soon, once I tidy it up 👍
bors added a commit that referenced this pull request Dec 7, 2018
Rollup of 7 pull requests

Successful merges:

 - #56000 (Add Armv8-M Mainline targets)
 - #56250 (Introduce ptr::hash for references)
 - #56434 (Improve query cycle errors for parallel queries)
 - #56516 (Replace usages of `..i + 1` ranges with `..=i`.)
 - #56555 (Send textual profile data to stderr, not stdout)
 - #56561 (Fix bug in from_key_hashed_nocheck)
 - #56574 (Fix a stutter in the docs for slice::exact_chunks)

Failed merges:

r? @ghost
@bors bors merged commit 0f47c2a into rust-lang:master Dec 7, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants