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

Initial support for building RISC-V runtimes targeting PolkaVM #3179

Merged
merged 9 commits into from
Feb 3, 2024

Conversation

koute
Copy link
Contributor

@koute koute commented Feb 2, 2024

This PR adds initial support for building RISC-V runtimes targeting PolkaVM.

  • Setting the SUBSTRATE_RUNTIME_TARGET=riscv environment variable will now build a RISC-V runtime instead of a WASM runtime.
  • This only adds support for building runtimes; running them will need a PolkaVM-based executor, which I will add in a future PR.
  • Only building the minimal runtime is supported (building the Polkadot runtime doesn't work yet due to one of the dependencies).
  • The builder now sets a substrate_runtime cfg flag when building the runtimes, with the idea being that instead of doing #[cfg(not(feature = "std"))] or #[cfg(target_arch = "wasm32")] to detect that we're building a runtime you'll do #[cfg(substrate_runtime)]. (Switching the whole codebase to use this will be done in a future PR; I deliberately didn't do this here to keep this PR minimal and reviewable.)
  • Further renaming of things (e.g. types, environment variables and proc macro attributes having "wasm" in their name) to be target-agnostic will also be done in a future refactoring PR (while keeping backwards compatibility where it makes sense; I don't intend to break anyone's workflow or create unnecessary churn).
  • This PR also fixes two bugs in the wasm-builder crate:
    • The RUSTC environment variable is now removed when invoking the compiler. This prevents the toolchain version from being overridden when called from a build.rs script.
    • When parsing the rustup toolchain list output the (default) is now properly stripped and not treated as part of the version.
  • I've also added a minimal CI job that makes sure this doesn't break in the future. (cc @paritytech/ci)

cc @athei


Also, just a fun little tidbit: quickly comparing the size of the built runtimes it seems that the PolkaVM runtime is slightly smaller than the WASM one. (production build, with the names section substracted from the WASM's size to keep things fair, since for the PolkaVM runtime we're currently stripping out everything)

  • .wasm: 625505 bytes
  • .wasm (after wasm-opt -O3): 563205 bytes
  • .wasm (after wasm-opt -Os): 562987 bytes
  • .wasm (after wasm-opt -Oz): 536852 bytes
  • .polkavm: 580338 bytes 550476 bytes (after enabling extra target features; I'll add those in another PR once we have an executor working)

@koute koute added R0-silent Changes should not be mentioned in any release notes I5-enhancement An additional feature request. labels Feb 2, 2024
@koute koute requested a review from a team February 2, 2024 05:42
@koute koute requested a review from a team as a code owner February 2, 2024 05:42
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Nice work!

.gitlab/pipeline/build.yml Outdated Show resolved Hide resolved
@@ -21,13 +21,16 @@ sp-api-proc-macro = { path = "proc-macro", default-features = false }
sp-core = { path = "../core", default-features = false }
sp-std = { path = "../std", default-features = false }
sp-runtime = { path = "../runtime", default-features = false }
sp-runtime-interface = { path = "../runtime-interface", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

I think we should directly import polkavm here. No need to pull in the entire sp-runtime-interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that this needs access to the polkavm_abi module we've defined in sp-runtime-interface, so just directly including the PolkaVM crates is not sufficient.

So I can think of three options here:

  1. Define the polkavm_abi module in sp-runtime-interface and depend on it here.
  2. Define the polkavm_abi in sp-wasm-interface (which actually isn't WASM specific, since PolkaVM support also uses what's defined in there because it reuses parts of the existing machinery to avoid having to rewrite everything, and the wasmtime feature for that crate is optional anyway; a better name for that crate would be, uh, sp-runtime-interface, but that's already taken, so maybe merging these two crates would be the way to go :P)
  3. Define the polkavm_abi in a new crate, say, sp-polkavm-interface, and depend on it here. (But that seems like an overkill to me to add a new crate for ~10 lines of code, and also because the stuff in sp-wasm-interface is also used it'd be weird to have WASM only use sp-wasm-interface but PolkaVM use both sp-polkavm-interface and sp-wasm-interface.)

What I would suggest we do here is:

  1. Leave the polkavm_abi definition in sp-runtime-interface and depend on it here.
  2. (In another PR) Remove the sp-wasm-interface crate and merge it with sp-runtime-interface. Then refactor sp-runtime-interface so that it's more modularized and you don't need to always pull in the whole thing.

Does this sound good to you? (Alternatively maybe you have a better idea?)

Copy link
Member

Choose a reason for hiding this comment

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

Ahh okay, yeah then fine to leave it the way you have done it right now.

Comment on lines +20 to +21
#[polkavm_derive::polkavm_define_abi(allow_extra_input_registers)]
pub mod polkavm_abi {}
Copy link
Member

Choose a reason for hiding this comment

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

What does that do? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a magic macro. (;

Basically, it defines the trait machinery required to automatically convert types on an FFI boundary. It essentially generates this:

pub mod polkavm_abi {
    pub trait IntoHost {
        type Regs;
        type Destructor;

        fn into_host(value: Self) -> (Self::Regs, Self::Destructor);
    }

    pub trait FromHost {
        type Regs;
        fn from_host(value: Self::Regs) -> Self;
    }

    #[doc(hidden)]
    pub mod private {
        /* ...more stuff... */
    }
}

The idea here is that you can define how the types are marshaled through the FFI boundary in one place, and then when you use the polkavm_import or polkavm_export macro you can give it a path to this module, and it will use the IntoHost and FromHost traits defined within to do the conversions. For example, something like this:

#[polkavm_derive::polkavm_define_abi]
pub mod my_abi {}

impl self::my_abi::FromHost for FancyType {
    type Regs = (u32, u32,); // It needs two registers to be marshaled through the FFI boundary.
    fn from_host(value: Self::Regs) -> Self { ... }
}

#[polkavm_derive::polkavm_import(abi = my_abi)]
extern {
    fn hostcall() -> FancyType;
}

fn get_fancy() {
    let fancy = unsafe { hostcall(); }
}

This is somewhat similar to the traits we already have in Substrate for converting into/from WASM FFI types, except it's part of the upstream instead of being Substrate-specific, and you can have multiple ABIs defined (e.g. you could have one for Substrate runtimes and another for smart contracts, and they won't conflict with each other).

substrate/utils/wasm-builder/src/lib.rs Outdated Show resolved Hide resolved
substrate/utils/wasm-builder/src/wasm_project.rs Outdated Show resolved Hide resolved
);
},
RuntimeTarget::Riscv => {
rustflags.push_str("-C target-feature=+lui-addi-fusion -C relocation-model=pie -C link-arg=--emit-relocs -C link-arg=--unique ");
Copy link
Member

Choose a reason for hiding this comment

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

I was hitting duplicate symbol name when implementing PolkaVM support for ink!. Essentially the call export was having problems. Is the --unique a solution for 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.

No, the --unique just allows LLD to emit multiple sections, which allows our linker to better optimize the blob in some cases.

Your problems with duplicate symbols are probably fixed in the newest version of the PolkaVM derive crate. With each successive version I've been coming up with less hacky ways to do imports/exports, and the way they're implemented right now manages to exclusively use rustc to generate the symbol names for us so there should not be any dupes anymore.

@koute koute added this pull request to the merge queue Feb 3, 2024
Merged via the queue into paritytech:master with commit e349fc9 Feb 3, 2024
127 checks passed
@koute koute deleted the master_polkavm branch February 3, 2024 04:59
- .docker-env
- .common-refs
script:
- SUBSTRATE_RUNTIME_TARGET=riscv cargo check -p minimal-runtime
Copy link
Member

Choose a reason for hiding this comment

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

Btw why does it only run check and not build?
I think there are possible errors that only arise on build.

Copy link
Member

Choose a reason for hiding this comment

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

The builder runs as part of the build.rs. It can not detect if you are running cargo check and thus, it will always compile. So, everything works here as expected.

bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…ytech#3179)

This PR adds initial support for building RISC-V runtimes targeting
PolkaVM.

- Setting the `SUBSTRATE_RUNTIME_TARGET=riscv` environment variable will
now build a RISC-V runtime instead of a WASM runtime.
- This only adds support for *building* runtimes; running them will need
a PolkaVM-based executor, which I will add in a future PR.
- Only building the minimal runtime is supported (building the Polkadot
runtime doesn't work *yet* due to one of the dependencies).
- The builder now sets a `substrate_runtime` cfg flag when building the
runtimes, with the idea being that instead of doing `#[cfg(not(feature =
"std"))]` or `#[cfg(target_arch = "wasm32")]` to detect that we're
building a runtime you'll do `#[cfg(substrate_runtime)]`. (Switching the
whole codebase to use this will be done in a future PR; I deliberately
didn't do this here to keep this PR minimal and reviewable.)
- Further renaming of things (e.g. types, environment variables and proc
macro attributes having "wasm" in their name) to be target-agnostic will
also be done in a future refactoring PR (while keeping backwards
compatibility where it makes sense; I don't intend to break anyone's
workflow or create unnecessary churn).
- This PR also fixes two bugs in the `wasm-builder` crate:
* The `RUSTC` environment variable is now removed when invoking the
compiler. This prevents the toolchain version from being overridden when
called from a `build.rs` script.
* When parsing the `rustup toolchain list` output the `(default)` is now
properly stripped and not treated as part of the version.
- I've also added a minimal CI job that makes sure this doesn't break in
the future. (cc @paritytech/ci)

cc @athei

------

Also, just a fun little tidbit: quickly comparing the size of the built
runtimes it seems that the PolkaVM runtime is slightly smaller than the
WASM one. (`production` build, with the `names` section substracted from
the WASM's size to keep things fair, since for the PolkaVM runtime we're
currently stripping out everything)

- `.wasm`: 625505 bytes
- `.wasm` (after wasm-opt -O3): 563205 bytes
- `.wasm` (after wasm-opt -Os): 562987 bytes
- `.wasm` (after wasm-opt -Oz): 536852 bytes
- `.polkavm`: ~~580338 bytes~~ 550476 bytes (after enabling extra target
features; I'll add those in another PR once we have an executor working)

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
@athei athei mentioned this pull request Apr 8, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. R0-silent Changes should not be mentioned in any release notes
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

4 participants