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

Update WebGPU #11737

Merged
merged 24 commits into from Jul 31, 2020
Merged

Update WebGPU #11737

merged 24 commits into from Jul 31, 2020

Conversation

kainino0x
Copy link
Collaborator

@kainino0x kainino0x commented Jul 28, 2020

Should be as close as possible to the top of tree webgpu.h while being compatible with the top of tree Dawn header.

This re-adds webgpu_cpp.{h,cpp} so they can be guaranteed to be compatible with Emscripten's webgpu.h.

The new mapping code has been specifically tested locally, and the rest was tested against a large graphics application that uses many (but certainly not all) of these features.

Reviewing each commit separately is recommended.

@kainino0x kainino0x force-pushed the webgpu-update branch 2 times, most recently from ba0b3e8 to 08f88c9 Compare July 28, 2020 23:49
- remove deprecated webgpu.h struct members no longer needed for padding
- update webgpu_compiletest
@kainino0x kainino0x marked this pull request as ready for review July 29, 2020 00:01
@kainino0x
Copy link
Collaborator Author

@austinEng PTAL! I recommend viewing each commit separately.

@juj @kripken FYI

@@ -1687,6 +1695,9 @@ def add_library(lib):
else:
add_library(system_libs_map['libsockets'])

if shared.Settings.USE_WEBGPU:
add_library(system_libs_map['libwebgpu_cpp'])
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 this should go on line 1681 (which is indented in)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's in the middle of sanitization stuff so I left it here but indented it properly.

system/lib/webgpu/webgpu_cpp.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@austinEng austinEng left a comment

Choose a reason for hiding this comment

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

Some initial comments; this might take a while to review

system/include/webgpu/webgpu_cpp.h Outdated Show resolved Hide resolved
src/library_webgpu.js Show resolved Hide resolved
@austinEng
Copy link
Contributor

Some initial comments; this might take a while to review

Actually, no additional comments now

@kainino0x
Copy link
Collaborator Author

@kripken I think this is ready to go. Would you like to re-review? (Please go ahead and land whenever ready.)

@kainino0x
Copy link
Collaborator Author

Hm, test_minimal_runtime_code_size failed, but I don't know what changed that.

src/library_webgpu.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@juj juj left a comment

Choose a reason for hiding this comment

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

lgtm, can address feedback in future PRs to keep iterating speed quick.

@kainino0x
Copy link
Collaborator Author

kainino0x commented Jul 31, 2020

I tried reproducing the test_minimal_runtime_code_size failure locally, but ./runner.py other.test_minimal_runtime_code_size passes even after merging with top-of-tree (on mac). This test actually has a negative delta on my machine, so maybe that's hiding the failure?

size of a.html == 13711, expected 13715, delta=-4 (-0.03%)

@kainino0x
Copy link
Collaborator Author

kainino0x commented Jul 31, 2020

hm, now I'm getting
size of a.html == 13711, expected 13711, delta=0
on both branches (maybe my negative delta was not after merging).

@kainino0x
Copy link
Collaborator Author

Hm, I guess merging with top-of-tree fixed the CI, or it got fixed separately.

@kainino0x kainino0x merged commit 25b3253 into emscripten-core:master Jul 31, 2020
@kainino0x
Copy link
Collaborator Author

Landing with two approvals, thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants