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

Enable RISCV #52787

Merged
merged 5 commits into from Aug 2, 2018

Conversation

Projects
None yet
10 participants
@dvc94ch
Copy link
Contributor

dvc94ch commented Jul 27, 2018

  • Enable LLVM backend.
  • Implement call abi.
  • Add built-in target riscv32imac-unknown-none.
  • Enable CI.

@dvc94ch dvc94ch referenced this pull request Jul 27, 2018

Merged

Implement __mulsi3. #251

@@ -373,6 +373,8 @@ supported_targets! {
("armv7-unknown-cloudabi-eabihf", armv7_unknown_cloudabi_eabihf),
("i686-unknown-cloudabi", i686_unknown_cloudabi),
("x86_64-unknown-cloudabi", x86_64_unknown_cloudabi),

("riscv32imac-unknown-none", riscv32imac_unknown_none),

This comment has been minimized.

@japaric

japaric Jul 27, 2018

Member

Perhaps the last part of the triple should be unknown-elf to match the triple used by the GCC tooling?

This comment has been minimized.

@dvc94ch

dvc94ch Jul 27, 2018

Author Contributor

I don't know if this is necessary. If it where something other than elf we'd have riscv32imac-unknown-(darwin|msvc).

This comment has been minimized.

@japaric

japaric Jul 27, 2018

Member

We'll be adding riscv64imac-linux-gnu in the future. I'd like to see some consistency in the triple format: $ARCH_PLUS_INSN_SET-$OS-$ENV_OR_FORMAT. *-unknown-none sounds like either the environment or the object format is none and that's not the case.

This comment has been minimized.

@dvc94ch

dvc94ch Jul 27, 2018

Author Contributor

I think the unknown refers to the cpu vendor, in case there are vendor specific bugs/optimizations/extensions. So is riscv32imac-unknown-none-elf ok?

This comment has been minimized.

@japaric

japaric Jul 27, 2018

Member

Sounds like the most future proof choice. 👍

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jul 27, 2018

Add built-in target riscv32imac-unknown-none.

We may want to bikeshed the name of the target before landing this. So far most (all?) the built-in targets use $ARCH$SUBARCH as the first component of their triple. AIUI, RISCV has a modular instruction set with the concept of base instruction sets (e.g. RV32I, RV64I) and instruction set extensions like M, A and C. These extensions directly map to LLVM target features (+m, +a, +c) and Rust target_feature, whereas the base instruction sets are close to LLVM architectures and Rust target_arch (riscv32 and riscv64). So, perhaps the first part of the triple should be riscv32i+mac to reflect that +mac are the extensions and riscv32i is the base instructions set (RV32I)? Or we could use the instruction set name that the RISCV spec uses: rv32imac, but then the word "riscv" won't appear in the target name.

I should also note that the RV32IMAC instruction set is the instruction set implemented in the SiFive Freedom E310 (the only RISCV silicon implementation I'm aware of (*)), and that @dvc94ch has been testing this target with that hardware for quite some time now.

(*) There's also the Freedom U540, a 64-bit multi-core RISCV SoC, but I don't know if anyone has one yet.

@japaric japaric added the WG-embedded label Jul 27, 2018

@japaric
Copy link
Member

japaric left a comment

Changes LGTM, but I'd like to see the last part of the target name to be changed to be more similar to the prefix used by the GCC toolchain (riscv32-unknown-elf / riscv64-unknown-elf).

I'll defer the decision of enabling the production of rust-std components right now and doing the final sign off to @alexcrichton.

r? @alexcrichton

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 27, 2018

Nice! To enable RISCV by default in LLVM I think that these lines will also want to change

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 27, 2018

For CI purposes, some standard questions we like to ask are:

  • How big is the LLVM backend for RISCV? Is it like hundreds of MB or just a few?
  • How stable has the LLVM backend been for RISCV? Does the standard library often need patches to work around deficiencies in the LLVM backend? Or is it pretty solid and rarely hitting asserts and such?

Other than that I'd just have the same naming questions @japaric is bringin up!

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jul 27, 2018

Does the standard library often need patches to work around deficiencies in the LLVM backend?

To clarify the target this PR is adding is a no_std target like the thumb targets so we would only be building core, alloc and compiler-builtins crates.

How stable has the LLVM backend been for RISCV?

It's an experimental backend like the WebAssembly backend. I think it has been in development for a longer time though (could be wrong).

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 27, 2018

Ah sorry I missed that! The question is similar though in the sense that the backend can handle all of libcore? It's true that WebAssembly is classified as "experimental" but it's been quite solid for our use cases at least, never regressing in something we added to libcore, only discovering bugs after the fact :)

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Jul 27, 2018

How big is the LLVM backend for RISCV? Is it like hundreds of MB or just a few?

Should be around or grow to the size of the mips backend.

How stable has the LLVM backend been for RISCV? Does the standard library often need patches to work around deficiencies in the LLVM backend? Or is it pretty solid and rarely hitting asserts and such?

The people working on the riscv llvm backend are diligent and knowledgeable. Since there were enough features to compile libcore I have rarely hit any asserts. I had more problems with debug info or missing features required for embedded development (unimplemented assembler directives and such). Debug info worked out of the box with the llvm from nightly, although I have not used it extensively yet. Since the c extension actually works now I'm getting instruction missaligned errors which I haven't fixed yet (in my bsp crates). openocd and gdb like to crash - don't know why.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 27, 2018

Ok great! Sounds like this is basically good to go from that perspective :)

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jul 28, 2018

How big is the LLVM backend for RISCV? Is it like hundreds of MB or just a few?

FWIW, I'm seeing an increase of 2MB (+3.33%) in librustc_codegen_llvm-llvm.so.

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jul 28, 2018

@dvc94ch do you know if this target is supposed to support CAS operations natively? The following program results in an LLVM error:

#![allow(warnings)]
#![crate_type = "lib"]
#![no_std]

use core::sync::atomic::{AtomicUsize, Ordering};

#[no_mangle]
pub fn foo(x: &AtomicUsize, y: usize) -> usize {
    x.swap(y, Ordering::SeqCst)
}
$ rustc -O --emit=obj --target riscv32imac-unknown-none riscv.rs
LLVM ERROR: Cannot select: 0x7f2e4c036840: i32,ch = AtomicSwap<(volatile load store seq_cst 4 on %ir.0)> 0x7f2e4c01f418, 0x7f2e4c036708, 0x7f2e4c0367d8
  0x7f2e4c036708: i32,ch = CopyFromReg 0x7f2e4c01f418, Register:i32 %0
    0x7f2e4c0366a0: i32 = Register %0
  0x7f2e4c0367d8: i32,ch = CopyFromReg 0x7f2e4c01f418, Register:i32 %1
    0x7f2e4c036770: i32 = Register %1
In function: foo

Is this an LLVM bug? Or is the target supposed to have atomic_cas set to false in its target specification?

(Codegen of atomic loads / stores works fine)

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Jul 28, 2018

I've seen that before: lowRISC/riscv-llvm#56 atomic support is incomplete. This also prevents porting libstd currently. Interesting to know that the issue hasn't been fixed yet.

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jul 28, 2018

Since the c extension actually works now I'm getting instruction missaligned errors which I haven't fixed yet (in my bsp crates)

You mean the "+c" feature, right? Do you think the implementation is buggy? Or are you using inline assembly and that could be the cause of the alignment problem?

If "+c" is buggy we could remove it for now. It only reduces the size of the binary, right? Unlike "+m", removing it shouldn't increase the number of intrinsics needed (e.g. __mulsi).

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jul 28, 2018

atomic support is incomplete.

In that case we could set atomic_cas to false for now. That would prevent users from using AtomicUsize's CAS API and hitting this LLVM error. We can enable it again once atomic support is fully implemented. @alexcrichton thoughts?

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Jul 28, 2018

I think it's my inline asm. But when I checked the spec it say's "The C extension is compatible with all other standard instruction extensions. The C extension allows 16-bit instructions to be freely intermixed with 32-bit instructions, with the latter now able to start on any 16-bit boundary." So it's not immediately clear what the issue might be. But I haven't had time to investigate it thoroughly yet.

EDIT: It works fine without the c extension
and another issue with the c extension is that the .no-relax directive isn't implemented yet to disable linker relaxation when the $gp is initialized - which might be the cause

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Jul 28, 2018

cc @asb riscv llvm maintainer

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Jul 28, 2018

Codegen for atomics has been committed about a month ago upstream, so it wasn't in LLVM 7 which we're using.

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Jul 28, 2018

Regarding the triple, I wish I could just argue for taking inspiration from the C toolchain conventions. Unfortunately, rustc's situation is sufficiently different -- no analogue to multilib -- that this might be difficult. So we're probably forced to ship a standard library for a specific set of extensions, with no way to

  • disable some of those extensions and get a standard library that doesn't use those restrictions either (e.g. to target an RV32I soft core), or
  • add more extensions and get a a standard library that uses the extra extensions as well (e.g., hardfloat)

So we're effectively shipping a toolchain tied to a particular set of extensions, rather than really presenting a "base ISA + pick your own extensions" view to the user. For this reason, I think it's fine to put something like rv32imac or riscv32imac into the target triple rather than pepper in +'s or otherwise pay lip service to the modularity of the ISA.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 28, 2018

We can enable it again once atomic support is fully implemented. @alexcrichton thoughts?

Seems reasonable to me @japaric!

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Jul 28, 2018

I disabled the c extension and atomic_cas. This commit is intended to be reverted in the future.

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Jul 29, 2018

@rkruppe

Codegen for atomics has been committed about a month ago upstream, so it wasn't in LLVM 7 which we're using.

In that case moving to the latest LLVM might fix. LLVM upgrades can be ... tricky so I'd suggest landing this now and do the LLVM upgrade in a separate PR.

presenting a "base ISA + pick your own extensions"

Pure MIR rlibs will let us tweak codegen (e.g. "+c", "+m") at the top level without changing the target and recompiling the whole dependency graph, but they won't help with features like "+a" which conditionally compile code like AtomicUsize.

I don't think we can have a single built-in rv32i-* target because some extensions like "+a" affect the API surface of core and one can't tweak the value of target_cas from the command line -- at that point you need a new target (specification).

Unfortunately, rustc's situation is sufficiently different -- no analogue to multilib -- that this might be difficult.

Off topic: multilib doesn't look that different to me.

$ # these are nameless targets
$ arm-none-eabi-gcc -print-multi-lib | grep v7e
thumb/v7e-m/nofp;@mthumb@march=armv7e-m@mfloat-abi=soft # this one is like our `thumbv7em-none-eabi` target
thumb/v7e-m+fp/softfp;@mthumb@march=armv7e-m+fp@mfloat-abi=softfp
thumb/v7e-m+fp/hard;@mthumb@march=armv7e-m+fp@mfloat-abi=hard
thumb/v7e-m+dp/softfp;@mthumb@march=armv7e-m+fp.dp@mfloat-abi=softfp
thumb/v7e-m+dp/hard;@mthumb@march=armv7e-m+fp.dp@mfloat-abi=hard

@mthumb@march=armv7e-m@mfloat-abi=soft is the target specification. In C it can be specified in the command line; in Rust it must be encoded in a target specification file.

/usr/arm-none-eabi/lib/thumb/v7e-m/nofp is an analogue to a rust-std component, pre-compiled core / std.


I disabled the c extension and atomic_cas. This commit is intended to be reverted in the future.

@alexcrichton @dvc94ch Alright this LGTM. I'm happy with the name too.

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Jul 30, 2018

In that case moving to the latest LLVM might fix.

I think these three patches are also required which haven't been reviewed yet. There is only one RISCV patch that isn't in rust's llvm, so there's no hurry.

https://reviews.llvm.org/D48131
https://reviews.llvm.org/D48130
https://reviews.llvm.org/D48129

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Jul 30, 2018

@japaric

I don't think we can have a single built-in rv32i-* target because some extensions like "+a" affect the API surface of core and one can't tweak the value of target_cas from the command line -- at that point you need a new target (specification).

Yes, in general we'd really need to compile the standard library on the user's machine with codegen options they choose (Xargo-style). Or a multilib-style setup...

Off topic: multilib doesn't look that different to me.

At least the way RISC-V GCC does it, users can add any extensions they like to their own compilation process and transparently get a working library that uses either exactly those features or a reasonably large subset of them (so there aren't exponentially many minor variations of the libraries on disk).

In contrast, while we can ship a libstd component for every set of features we like, users will always have to pick one of those manually and specify it everywhere.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 30, 2018

@dvc94ch do you want to merge this before the LLVM patches land? It's fine from our perspective to do so I believe and we can always update LLVM later once the patches land.

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Jul 30, 2018

@alexcrichton Sounds good to me!

dvc94ch added some commits Jul 24, 2018

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Aug 1, 2018

ping @kennytm

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Aug 1, 2018

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 1, 2018

📌 Commit d974dc9 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 1, 2018

⌛️ Testing commit d974dc9 with merge 3452276...

bors added a commit that referenced this pull request Aug 1, 2018

Auto merge of #52787 - riscv-rust:riscv-rust-pr, r=alexcrichton
Enable RISCV

- Enable LLVM backend.
- Implement call abi.
- Add built-in target riscv32imac-unknown-none.
- Enable CI.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 1, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Aug 1, 2018

The job dist-various-1 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)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 1, 2018

@bors: retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 2, 2018

⌛️ Testing commit d974dc9 with merge 60c1ee7...

bors added a commit that referenced this pull request Aug 2, 2018

Auto merge of #52787 - riscv-rust:riscv-rust-pr, r=alexcrichton
Enable RISCV

- Enable LLVM backend.
- Implement call abi.
- Add built-in target riscv32imac-unknown-none.
- Enable CI.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 2, 2018

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

@bors bors merged commit d974dc9 into rust-lang:master Aug 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@zarvox

This comment has been minimized.

Copy link

zarvox commented Aug 5, 2018

I have one of the Freedom U540 boards -- feel free to ping me or CC me on things if you want me to test something out!

@japaric

This comment has been minimized.

Copy link
Member

japaric commented Aug 7, 2018

@zarvox Nice! Atomic support is not complete in the version of LLVM we are using so the AtomicUsize API is not complete and std can't be built. Thus we can't build Linux (std) programs at the moment. When it becomes possible to build Linux programs for RISCV we'll let you know.

@dvc94ch

This comment has been minimized.

Copy link
Contributor Author

dvc94ch commented Aug 7, 2018

FYI: I probably won't be doing this work since a U540 board costs around 1000$ and I'm more interested in rust on riscv soft cores at the moment.

@asb

This comment has been minimized.

Copy link

asb commented Aug 7, 2018

Hi, RISC-V LLVM code owner here. Many thanks to @dvc94ch and everyone else who has contributed to enabling RISC-V support in Rust. I'd love to see support for Rust on RISC-V Linux targets too, though it's worth noting there are other pre-requisites beyond the atomics support that isn't fully merged (e.g. hardfloat ABI, PIC, TLS). We should make better use of the upstream LLVM bugzilla to keep track of unimplemented features that are useful to the Rust community (e.g. the .option push/pop mentioned in this thread).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.