Skip to content

WebGPU: roll header and generated changes in from Dawn #20880

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

Merged
merged 13 commits into from
Jan 18, 2024

Conversation

kainino0x
Copy link
Collaborator

@kainino0x kainino0x commented Dec 8, 2023

Rolls to dawn revision:
https://dawn.googlesource.com/dawn/+log/ad853f80af9b236fa3aa6be6dcd2facb26430565
except skips a few entrypoints that can't be implemented in JS (require a C++ shim)
and fixes library_webgpu.js where necessary (function stubs, several of which are going to change before being implemented)

Also adds several files to match Dawn, so that Emscripten mirrors Dawn's generated files directly, requiring less manual intervention, but there is more work to do here.

@PZerua
Copy link

PZerua commented Dec 11, 2023

I've noticed Chrome expects "float32-filterable" instead of "float32filterable". If I change it manually in library_webgpu.js I can sample float32 textures on web with no problem. Could this be incorporated in this PR?

@kainino0x
Copy link
Collaborator Author

I've noticed Chrome expects "float32-filterable" instead of "float32filterable". If I change it manually in library_webgpu.js I can sample float32 textures on web with no problem. Could this be incorporated in this PR?

Nice catch! Yes, I'll incorporate it into that Dawn change and this PR.

Rolls to dawn revision:
https://dawn.googlesource.com/dawn/+log/ad853f80af9b236fa3aa6be6dcd2facb26430565
and fixes library_webgpu.js where necessary.

Also adds several files to match Dawn, so that Emscripten mirrors Dawn's
generated files directly, requiring less manual intervention.
@kainino0x kainino0x marked this pull request as ready for review January 13, 2024 02:56
@kainino0x
Copy link
Collaborator Author

@beaufortfrancois PTAL

@kainino0x kainino0x requested a review from sbc100 January 13, 2024 04:11
These functions can't be implemented in JS because the signatures are
too long. They need to be implemented with a C(++) shim over JS but this
will happen in a forthcoming PR.
@kainino0x
Copy link
Collaborator Author

These instructions are truly out of hand. I will work on this soon :)

I also had to revert some small bits to fix failing compile tests:

Remove functions that have too-long signatures
These functions can't be implemented in JS because the signatures are
too long. They need to be implemented with a C(++) shim over JS but this
will happen in a forthcoming PR.

@beaufortfrancois PTAL

@kainino0x
Copy link
Collaborator Author

@sbc100 PTAL as well, I think this is very close to ready to land.

@kainino0x
Copy link
Collaborator Author

Oh I just realized François has review privileges here, so maybe that won't be necessary?

Copy link
Contributor

@beaufortfrancois beaufortfrancois left a comment

Choose a reason for hiding this comment

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

LGTM with nits

@@ -1227,6 +1227,12 @@
"size",
"mappedAtCreation"
],
"WGPUBufferMapCallbackInfo": [
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this should not be there because MapAsyncF is not as well?
Does this means all Future related stuff below should be removed also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I chose not to remove those things adding them back will increase the size of the next roll. So I just removed the minimum needed to make it build (I did this manually, not in dawn.json).

"WGPUFuture": [
"id"
],
"WGPUInstanceFeatures": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this implemented in Emscripten?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, not yet, but it will be soon.

'pending',
'mapped',
],
BufferMapState: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use leading undefined in some cases about but map in other cases like this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quirk of the code generator, it's because CompareFunction[0] is expected to be used ever while BufferMapState[0] is not. I was thinking of smoothing this over in the generator along with other generation cleanups (like hopefully moving some generated code into separate files so we can get rid of "please copy-paste" steps)

@@ -0,0 +1,161 @@
// Copyright 2017 The Dawn & Tint Authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the date here correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The comment hasn't been updated in a long time but I'll fix this upstream in Dawn first.

#include "webgpu/webgpu_cpp.h"

#ifdef __GNUC__
#if defined(__GNUC__) || defined(__clang__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, clang defines __GNUC__ so this should not be needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will propose this change to Dawn first as well.

@@ -1,6 +1,9 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing copyright header?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Huh, this needs to be fixed upstream. In the meantime let me add the header we have in the template since this one is more important.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if it really is from 2017 then this is fine.. I didn't realize webgpu went back that far :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, and this the enum class bitmasks utility probably hasn't even changed much since then. tbd if I update that.

The generated headers will have 2024 in them because the date is written at generation time, so I put those in as 2024.

@kainino0x kainino0x enabled auto-merge (squash) January 17, 2024 19:49
@kainino0x kainino0x disabled auto-merge January 17, 2024 23:22
@kainino0x kainino0x enabled auto-merge (squash) January 18, 2024 01:13
@kainino0x kainino0x merged commit 8915f2b into emscripten-core:main Jan 18, 2024
@kainino0x kainino0x deleted the dawn-roll branch January 18, 2024 06:24
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.

4 participants