-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Roll Emdawnwebgpu and repoint USE_WEBGPU tests at Emdawnwebgpu #25397
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
base: main
Are you sure you want to change the base?
Conversation
In preparation to remove USE_WEBGPU entirely (emscripten-core#24265). Ideally these tests would live in Dawn, but we don't have a way to automate tests in the browser yet, so for the moment we'll keep them in Emscripten. DO NOT SUBMIT: Needs Emdawnwebgpu roll in order to pass
@lokokung PTAL |
a88588a
to
ffb124d
Compare
ffb124d
to
d12517b
Compare
3b2d73d
to
4c7bf8a
Compare
4c7bf8a
to
4f93d5f
Compare
webgpu.cpp uses C++ library functions, so it needs to be linked with C++ support, otherwise it gets a long list of missing C++ STL symbol errors. Make this an explicit error so people know what to do if they get this error (because they're using -sSTRICT or -sNO_DEFAULT_TO_CXX). Initial discussion: emscripten-core/emscripten#25397 (comment) Bug: none Change-Id: I99b766298f47b946e0ba49eacad04fe8b427f6de Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/264434 Reviewed-by: Loko Kung <[email protected]> Commit-Queue: Kai Ninomiya <[email protected]>
…sn't release until after the callback)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really qualified to read the .c
/.cpp
changes to the test files, so I'll have to take your word for it mostly that they good.
@juj might have some opinions?
'-Wno-error=deprecated', | ||
'-sINCLUDE_FULL_LIBRARY', | ||
'-sUSE_WEBGPU', | ||
'--use-port=emdawnwebgpu', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any remaining tests for -sUSE_WEBGPU
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but I'm planning to remove it immediately after this (#25398).
'dylink': (['-sMAIN_MODULE'],), | ||
}) | ||
def test_webgpu_compiletest(self, args): | ||
self.run_process([EMXX, test_file('webgpu_jsvalstore.cpp'), '-Wno-error=deprecated', '-sUSE_WEBGPU', '-sASYNCIFY'] + args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test no longer useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole jsvalstore thing has been removed in Emdawnwebgpu. We have another mechanism but it would need entirely new tests.
self.run_process([EMXX, test_file('test_emdawnwebgpu_link_test.cpp'), '--use-port=emdawnwebgpu', '-sASYNCIFY'] + args) | ||
if config.FROZEN_CACHE: | ||
# TODO(crbug.com/446944885): Make Emdawnwebgpu work with FROZEN_CACHE if possible. | ||
self.skipTest("test doesn't work with frozen cache") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this skipTest
needed now but not before this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because previously the cache was pre-populated with -sUSE_WEBGPU
but we don't prepopulate it with --use-port=emdawnwebgpu
(which would have to be done with several different flags too).
I can try to pre-populate the cache with the appropriate Emdawnwebgpu objects if you think that's appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 103 to 104 in 3b22cd6
'libwebgpu', | |
'libwebgpu_cpp', |
I think it comes from this? I don't think it'll be that easy to add Emdawnwebgpu here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it would be added via this
Line 139 in 3b22cd6
PORTS = sorted(list(ports.ports_by_name.keys()) + list(ports.port_variants.keys())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the old code and the new code here both use --use-port=emdawnwebgpu
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I missed which test this was. I actually have no idea. If I remove emdawnwebgpu from my local cache and run the old test (on main
), it still fails with FROZEN_CACHE is set, but cache file is missing
.
Could the @flaky
be suppressing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, CI has an explicit separate Embuilder step. Let me figure out how to test that locally and then see where I need to skip tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I figured out which configurations don't get precached and skipped only those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... Although I did not figure out exactly why it was passing before and isn't now. @also_with_wasm64
isn't newly-added.
LGTM with the test changes - looks like they're effectively refactoring callbacks to lambdas. |
Mainly yes. But also taking advantage of keepalives to let a bunch of async stuff finish before the test completes (without using asyncify). |
2b69827
to
8ebf5ef
Compare
In preparation to remove USE_WEBGPU entirely (#24265). Ideally these tests would live in Dawn, but we don't have a way to automate tests in the browser yet, so for the moment we'll keep them in Emscripten.
Emdawnwebgpu roll includes:
bb3dc45 [emscripten] Use Emscripten `diagnostics.error` instead of `raise`
a3650c0 [emscripten] Error on LINK_AS_CXX=0 in emdawnwebgpu.port.py