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

Use official emsdk bazel toolchain #4769

Merged
merged 30 commits into from
Mar 30, 2021

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented Mar 2, 2021

Build tfjs-backend-wasm with the official emsdk toolchain open-sourced from google3.

Using the official toolchain fixes the issue where normal, simd, and simd + threaded builds write their artifacts to the same output location, invalidating the build cache. With this change, outputs are created in separate folders, enabling build caching.

Additionally, the emscripten toolchain is now automatically downloaded as a dependency within Bazel, so it no longer needs to be installed separately.

A minor edit to the official toolchain was needed to make simd work correctly with XNNPACK.

This PR also enables remote build caching for the wasm backend on CI runs.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@google-cla google-cla bot added the cla: yes label Mar 2, 2021
@mattsoulanille mattsoulanille changed the title Wasm toolchain 2.0.14 Use official emsdk bazel toolchain Mar 2, 2021
copybara-service bot pushed a commit to google/XNNPACK that referenced this pull request Mar 4, 2021
…en toolchain.

Current Bazel config settings for Emscripten are specific to TensorFlow.js's custom Emscripten toolchain, but TFJS is switching to the official toolchain in [#4769](tensorflow/tfjs#4769). The official emscripten toolchain expects `crosstool_top` to be `//emscripten_toolchain:everything`.

PiperOrigin-RevId: 360784362
copybara-service bot pushed a commit to google/XNNPACK that referenced this pull request Mar 4, 2021
…en toolchain.

Current Bazel config settings for Emscripten are specific to TensorFlow.js's custom Emscripten toolchain, but TFJS is switching to the official toolchain in [#4769](tensorflow/tfjs#4769). The official emscripten toolchain expects `crosstool_top` to be `//emscripten_toolchain:everything`.

PiperOrigin-RevId: 360784362
copybara-service bot pushed a commit to google/XNNPACK that referenced this pull request Mar 4, 2021
…en toolchain.

Current Bazel config settings for Emscripten are specific to TensorFlow.js's custom Emscripten toolchain, but TFJS is switching to the official toolchain in [#4769](tensorflow/tfjs#4769). The official emscripten toolchain expects `crosstool_top` to be `//emscripten_toolchain:everything`.

PiperOrigin-RevId: 360787196
@mattsoulanille
Copy link
Member Author

mattsoulanille commented Mar 9, 2021

Remote caching is not working in GCP yet, but is working with a locally run 'remote' cache. I'm sending this out for review now, but we might want to delay merging it until after the remote cache is working on GCP (which I'm pretty sure just requires some GCP config changes but might require some changes to this PR as well).

@mattsoulanille
Copy link
Member Author

Note to reviewers: Everything in emscripten_toolchain/ is from the emsdk Bazel toolchain with a minor edit. Also, the new toolchain required a few changes to the cc source files, mostly involving removing unused variables. These are detailed in remove unused variables.

@mattsoulanille mattsoulanille marked this pull request as ready for review March 9, 2021 23:14
Copy link
Collaborator

@jinjingforever jinjingforever left a comment

Choose a reason for hiding this comment

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

This is super cool! Thank you so much for making this work!

Reviewed 6 of 34 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128, @mattsoulanille, and @pyu10055)


.bazelrc, line 23 at r2 (raw file):

to so

nit: typo


emscripten_toolchain/emar.sh, line 1 at r1 (raw file):

#!/bin/bash

Do we need to add the license text to these files?

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Thanks Matt, this is great. A high level question, is your change to the toolchain upstreamed to the repo? should we maintain a copy of the emscripten_toolchain files or be downloaded as needed from the repo?

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128 and @mattsoulanille)

Copy link
Member Author

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

My change isn't upstreamed since I'm pretty sure it's specific to just xnnpack. We should probably maintain our own copy of emscripten_toolchain, but we could set up a copybara to automatically pull new changes and apply the changes I made.

I also changed emscripten's npm_install name to avoid a conflict with our monorepo's yarn_install, so we'd need to use copybara even if the change were upstreamed.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @jinjingforever and @lina128)


.bazelrc, line 23 at r2 (raw file):

Previously, jinjingforever (Jing Jin) wrote…
to so

nit: typo

Thanks!


emscripten_toolchain/emar.sh, line 1 at r1 (raw file):

Previously, jinjingforever (Jing Jin) wrote…

Do we need to add the license text to these files?

I don't think so since they're pulled directly from emsdk per their setup instructions.

@mattsoulanille
Copy link
Member Author

mattsoulanille commented Mar 10, 2021

I tried this on my mac, and I think this PR may break support for compiling wasm on OSX and Windows. It looks like the build rules only ship with Linux binaries for emscripten. I'll check with the author.

Edit: looks like there's an issue open for this: emscripten-core/emsdk#650

Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Great work!!! Just curious, if we are going to sync wasm backend to g3 and allows it to build in g3, do we also use these toolchain?

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @jinjingforever)

Copy link
Member Author

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

I think we can build in g3 using g3 rules. They're very similar to the open source ones. I'll give it a try.

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @jinjingforever)

@mattsoulanille
Copy link
Member Author

mattsoulanille commented Mar 29, 2021

Support for operating systems other than linux has been fixed in emsdk, and the way the toolchain is loaded has been significantly simplified (it's now loaded as an external repository instead of copied into our workspace).

I've tested this PR on my mac again, and it now compiles and passes the tests. I think it's ready to be merged, but PTAL again. Thanks!

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Thanks! Cannot wait to see it in action.

@lina128
Copy link
Collaborator

lina128 commented Mar 30, 2021

Great work, thank you Matt!

@mattsoulanille mattsoulanille merged commit 6295d6b into tensorflow:master Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants