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 wasm LLVM backend #42571

Merged
merged 1 commit into from
Jun 20, 2017
Merged

Enable wasm LLVM backend #42571

merged 1 commit into from
Jun 20, 2017

Conversation

tlively
Copy link
Contributor

@tlively tlively commented Jun 9, 2017

Enables compilation to WebAssembly with the LLVM backend using the target triple "wasm32-unknown-unknown". This is the beginning of my work on #38804.

edit: The new new target is now wasm32-experimental-emscripten instead of wasm32-unknown-unknown.

@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 @nikomatsakis (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.

@nikomatsakis
Copy link
Contributor

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned nikomatsakis Jun 9, 2017
@eddyb
Copy link
Member

eddyb commented Jun 9, 2017

You have a merge commit: you should always pass --rebase to git pull.

@brson
Copy link
Contributor

brson commented Jun 9, 2017

Thanks @tlively! I'm amazed you've made so much progress. I'm not going to look at this tonight, but next monday.

dynamic_linking: false,
executables: true,
// Today emcc emits two files - a .js file to bootstrap and
// possibly interpret the wasm, and a .wasm file

Choose a reason for hiding this comment

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

Perhaps it'll be better to use WebAssembly Standalone mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that we should not always produce JavaScript, and I would like to be able to get there eventually. However, we probably want to produce the JavaScript file at least when compiling executables, since wasm is not executable without a JavaScript wrapper to launch it. When compiling libraries, emitting only wasm makes much more sense, but we have to make sure we can do that in a way that will allow them to be linked and used naturally, and in a way that does not strengthen our dependency on emscripten's LLVM modifications. I plan to do this work in a future PR.

Choose a reason for hiding this comment

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

I'm not arguing against producing JavaScript along with wasm but you always can wrap you wasm by hands and have customization that produced JS doesn't provide.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2017
@frewsxcv frewsxcv added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2017
@@ -94,6 +94,7 @@ pub fn llvm(build: &Build, target: &str) {
.profile(profile)
.define("LLVM_ENABLE_ASSERTIONS", assertions)
.define("LLVM_TARGETS_TO_BUILD", llvm_targets)
.define("LLVM_EXPERIMENTAL_TARGETS_TO_BUILD", "WebAssembly")
Copy link
Contributor

Choose a reason for hiding this comment

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

See how LLVM_TARGETS_TO_BUILD can be configured by config.llvm_targets? I think we better do similar by adding config.llvm_experimental_targets - it doesn't seem right to allow all the targets to be configured out except the experimental one. It should be reasonably obvious how to make that change, but it will also require updating the config file example.

@@ -214,6 +214,7 @@ supported_targets! {
("le32-unknown-nacl", le32_unknown_nacl),
("asmjs-unknown-emscripten", asmjs_unknown_emscripten),
("wasm32-unknown-emscripten", wasm32_unknown_emscripten),
("wasm32-unknown-unknown", wasm32_unknown_unknown),
Copy link
Contributor

Choose a reason for hiding this comment

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

Using "unknown" for the OS here is a bit odd, since this platform does link with emcc and runs on the emscripten runtime. I think, as we discussed previously, this target is destined to replace the existing 'wasm32-unknown-emscripten' target. Do you see this name as temporary?

Copy link
Contributor Author

@tlively tlively Jun 13, 2017

Choose a reason for hiding this comment

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

wasm32-unknown-unknown is the triple that LLVM uses for its WebAssembly backend, and it will eventually be possible to link wasm with lld. If the Emscripten runtime were to be replaced with a smaller Rust-specific runtime, we could remove all dependencies on Emscripten for WebAssembly, which I think would be ideal. Given that, I wasn't thinking of wasm32-unknown-unknown as temporary, although another possible name for an Emscripten-free target would be wasm32-unknown-wasm.

On the other hand, this could be a temporary name, and if we decide not to replace the Emscripten linker and runtime then it would make sense to rename this target to replace the old one once it is ready. Another option would be to eventually have different targets for each of the possible wasm paths: fastcomp-emscripten (the current path), llvm-emscripten (this PR), and llvm-lld.

In the meantime, it might make sense for me to rename the target in this PR wasm32-unknown-experimental, since it really is not yet a usable target.

cc @eholk

Copy link

@cretz cretz Jun 13, 2017

Choose a reason for hiding this comment

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

To add, WASM supports non-web target, and as a developer of an alternate wasm backend myself, the emscripten-free target is ideal.

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case I think this name is fine for now.

In the Rust system I would expect an emscripten-wasm target and a non-emscripten-wasm target to have two different target triples.

@@ -81,6 +81,7 @@ static TARGETS: &'static [&'static str] = &[
"s390x-unknown-linux-gnu",
"sparc64-unknown-linux-gnu",
"wasm32-unknown-emscripten",
"wasm32-unknown-unknown",
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not necessary.

This list is defining the targets we distribute as part of Rust releases, and this target will not be built and distributed at this time.

@brson
Copy link
Contributor

brson commented Jun 12, 2017

@tlively This looks like a fine start to me. Thanks a bunch. I left some comments to be addressed.

Can you say a bit about what your next steps are? I believe you are going to be experimenting with this backend out of tree, probably with an upgraded LLVM, and slowly upstreaming Rust changes that can be upstreamed until you are ready to do the big LLVM upgrade.

Since this backend is not useful generally right now, it could be worth making the "llvm_experimental_targets" empty by default and requiring it to be configured explicitly. We are configuring in by default things like PTX and hexagon, surely nobody is doing anything productive with PTX today.

Yeah, I think I am inclined to make the wasm backend off by default and require devs (you :)) to configure it in explicitly. Will you make that change?

@alexcrichton can you peek at this and see what you think?

@brson
Copy link
Contributor

brson commented Jun 12, 2017

I guess I shouldn't say it's not generally useful. I just assume that. Does this target produce working wasm using the in-tree LLVM today?

@brson
Copy link
Contributor

brson commented Jun 12, 2017

Please rebase and squash these commits.

@tlively
Copy link
Contributor Author

tlively commented Jun 13, 2017

I guess I shouldn't say it's not generally useful. I just assume that. Does this target produce working wasm using the in-tree LLVM today?

It does, with some big caveats. It produces usable #[no_core] binaries, and with some effort produces usable #[no_std] binaries. With more effort it can produce binaries linked with std, but due to a version mismatch somewhere in my toolchain these are broken. I would definitely describe this as "not generally useful."

it could be worth making the "llvm_experimental_targets" empty by default and requiring it to be configured explicitly.

This sounds like a good idea. I will submit a separate PR that makes this change for existing experimental targets, and once/if that lands I will update this PR as well.

Can you say a bit about what your next steps are? I believe you are going to be experimenting with this backend out of tree, probably with an upgraded LLVM, and slowly upstreaming Rust changes that can be upstreamed until you are ready to do the big LLVM upgrade.

That sounds about right, except that I'm going to try using the in-tree LLVM backend as long as possible. I will probably have to do an upgrade at some point, but since my impression is that that could be difficult, I'm trying to implement as much new upstreamable functionality as possible first.

My immediate next steps will be to get tests building and running and make sure the #[no_core] tests pass. I will also clean up or document assumptions the build system has about the environment to build rustc with this target enabled. After that I will try to get the rest of the tests to pass, which may involve updating LLVM. Once the tests pass, I will be able to work on removing the Emscripten linking and runtime dependency if that's something we want to do.

@arielb1 arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2017
@tlively
Copy link
Contributor Author

tlively commented Jun 15, 2017

ping @brson, this is ready for another look. Should PRs always be squashed to one commit, or was it only because of merge commits and other dirtiness that the original commits had to be squashed?

@tlively
Copy link
Contributor Author

tlively commented Jun 16, 2017

It turns out that changing the name of the target to wasm32-experimental-emscripten means that I automatically pick up all the special cases for the emscripten target everywhere else in the code base, so hold on for an update for that.

The new target is wasm32-experimental-emscripten. Adds a new
configuration option to opt in to building experimental LLVM backends
such as the WebAssembly backend. The target name was chosen to be
similar to the existing wasm32-unknown-emscripten target so that the
build and tests would work with minimal other code changes. When/if the
new target replaces the old target, simply renaming it should just work.
@tlively
Copy link
Contributor Author

tlively commented Jun 16, 2017

ping @brson. This should be good to go now. The next steps will be to get testing working without all the manual intervention needed now, then possibly to create a bot for this target. Getting all the tests to pass may involve updating LLVM and fixing bugs in the backend.

@SpaceManiac
Copy link

There seems to be some disagreement about target triple in the code. Is it wasm32-unknown-unknown (opening comment), wasm32-experimental-emscripten (filename and registration), or wasm32-unknown-emscripten (the Target struct)?

@tlively
Copy link
Contributor Author

tlively commented Jun 19, 2017

Is it wasm32-unknown-unknown (opening comment), wasm32-experimental-emscripten (filename and registration), or wasm32-unknown-emscripten (the Target struct)?

The Rust target this PR introduces is wasm32-experimental-emscripten (in a previous iteration it was wasm32-unknown-unknown). The target triple that gets passed to LLVM is still wasm32-unkown-unknown, because that is what LLVM knows about. There is also still the other wasm target, wasm32-unknown-emscripten, that uses Emscripten for code generation. The new experimental target will probably be renamed to replace the wasm32-unknown-emscripten target once it is stable and good enough.

@eholk
Copy link
Contributor

eholk commented Jun 19, 2017

@brson or @alexcrichton - could you take another look at this?

@alexcrichton
Copy link
Member

Sorry for the delay, looks great @tlively!

@bors: r+

FWIW we recently added a disabled builder for wasm32-unknown-emscripten, so perhaps this could reuse and/or copy a lot of that for a wasm32-experimental-emscripten target? We unfortunately have capacity issues right now so we're not in a position to add more builders, but that shouldn't stop you making them!

@bors
Copy link
Contributor

bors commented Jun 19, 2017

📌 Commit a1981a6 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jun 20, 2017

⌛ Testing commit a1981a6 with merge 380100c...

bors added a commit that referenced this pull request Jun 20, 2017
Enable wasm LLVM backend

Enables compilation to WebAssembly with the LLVM backend using the target triple "wasm32-unknown-unknown". This is the beginning of my work on #38804.

**edit:** The new new target is now wasm32-experimental-emscripten instead of wasm32-unknown-unknown.
@bors
Copy link
Contributor

bors commented Jun 20, 2017

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

@bors bors merged commit a1981a6 into rust-lang:master Jun 20, 2017
@eholk
Copy link
Contributor

eholk commented Jun 23, 2017

FYI @vadimcn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.