Enable WebGPU backend for Emscripten builds#609
Enable WebGPU backend for Emscripten builds#609j8asic wants to merge 33 commits intoshader-slang:mainfrom
Conversation
|
|
skallweitNV
left a comment
skallweitNV
left a comment
There was a problem hiding this comment.
Thanks for the submission. Overall this looks like it's going into the right direction.
FYI, the WebGPU backend in slang-rhi is very much a work-in-progress and very low priority for us at this point in time. We don't expect anything apart from the most trivial examples to run.
If we want to add support for WASM to slang-rhi, we need to add proper testing through CI. My main fear is that we get incompatible versions of Dawn and emdawnwebgpu. Do you know how emdawnwebgpu is versioned? Is it just implicitly versioned through the emscripten version?
understandable, that's why i'd love to help
This is a pain. Current slang dawn version is old (april 2025?), and seems that emscripten changed versioning from that point. In any case, for now I just decided emscripten is using "newer" webgpu api. There are few cases that have preprocessor checks dawn/emscripten to decide the api call. I see few options to enhance it: use template magic to auto-deduce right api call, or dig a bit on connecting versioning emscripten to dawn (but this would make sense only when Slang updates a bit its dawn version?). Going to think about CI tests now as well... |
skallweitNV
left a comment
skallweitNV
left a comment
There was a problem hiding this comment.
Taken another look. I think it's fine to merge initial WASM support with a little bit of additional cleanup.
src/wgpu/wgpu-api.h
Outdated
|
|
||
| #define SLANG_RHI_WGPU_PROCS(x) \ | ||
| x(AdapterInfoFreeMembers) \ | ||
| // Define native-only procedures that are not available or different in Emscripten |
There was a problem hiding this comment.
I don't really like this separation because I currently use the tools/extract-webgpu-procs.py script to generate the SLANG_RHI_WGPU_PROCS(x) definition whenever we update Dawn. This will make this tedious.
Maybe just define all of SLANG_RHI_WGPU_PROCS for both the WASM and Dawn case.
There was a problem hiding this comment.
This is the main issue for maintenance. I iterated two-three different solutions and found a compromise as you say to "define all of SLANG_RHI_WGPU_PROCS": since you already have a workflow, I extended your script to fetch (according commit of the) EM webgpu.h, and use it to typedef missing stubs.
There was a problem hiding this comment.
Why can't we extract two sets of stubs, one from the currently used Dawn library, the other from the webgpu headers? Maybe that's the wrong approach, but I'm not sure what mixing the two buys us?!
There was a problem hiding this comment.
Ok, to be straightforward the py script now explicitly defines two sets of procs. Em list is loaded from statically linkage, Dawn dynamically. Initial tests for creating the adapter, device, queue work nicely
src/wgpu/wgpu-api.h
Outdated
|
|
||
| #define SLANG_RHI_WGPU_PROCS(x) \ | ||
| x(AdapterInfoFreeMembers) \ | ||
| // Define native-only procedures that are not available or different in Emscripten |
There was a problem hiding this comment.
Why can't we extract two sets of stubs, one from the currently used Dawn library, the other from the webgpu headers? Maybe that's the wrong approach, but I'm not sure what mixing the two buys us?!
|
@j8asic sorry for not responding earlier. Thanks for addressing the review notes. I think the remaining open question is regarding the WebGPU API entrypoints. I'd prefer defining the stubs twice, one set for Emscripten, the other for Dawn. The script can then just simply iterate over the two headers and emit a set of stubs for each header. This will make diffs cleaner and generally avoid confusion between the two sets of entry points. Maybe I'm missing something obvious here, but this just seems the simpler more straight forward approach to me than extracting a common set. Did your change to ship WASM binaries for slang releases already land? If so, it would be neat to clean this up as well. Let me know if we need a new Slang release after your changes have landed. |
I thought about this option as well, but went with the current implementation, because Dawn impl assures it implements webgpu.h of emdawngpu + its own extensions; see 1 2. This is also connected to your initial worry on how to define syncing of API versions of dawn and emdawn. I had to manually check em releases compared to current chosen Dawn version. Let me know on your final decision and I will implement any.
This got merged after 2026.01, so I similarly suggest we wait for new release containing wasm-libs and clean this up. |
|
Added explicit distinction of dawn/em API to be loaded, and tests are running nicely. N.b. I have not created any html tests in the repo, to be run. If you made a decision on how to approach automatic testing let me know. Please re-review, IMHO this can now be merged, as we are running late with WASM-libs PR (shader-slang/slang#9818). When done, I will add auto fetching from Slang releases. Btw I have SGL almost running via WASM |
|
@j8asic sorry for not responding earlier. I will take another look tomorrow hopefully. Thanks for all the effort on this and nice to hear SGL almost works in WASM :) |
|
SGL now works in browsers... |
skallweitNV
left a comment
skallweitNV
left a comment
There was a problem hiding this comment.
LGTM. Some suggestions. I think especially the code duplication should be reduced unless there are slight differences that I didn't notice.
src/wgpu/wgpu-buffer.cpp
Outdated
|
|
||
| #include "core/deferred.h" | ||
|
|
||
| #ifdef SLANG_WASM |
There was a problem hiding this comment.
Instead of splatting the <emscripten.h> include everywhere, we could just put it into wgpu-api.h once if we need emscripten API in the WASM case in different places.
src/wgpu/wgpu-buffer.cpp
Outdated
| #if SLANG_WASM | ||
| uint64_t timeoutNS = 0; | ||
| WGPUWaitStatus waitStatus = WGPUWaitStatus_Success; | ||
| while (true) | ||
| { | ||
| waitStatus = m_ctx.api.wgpuInstanceWaitAny(m_ctx.instance, SLANG_COUNT_OF(futures), futures, timeoutNS); | ||
| if (futures[0].completed) | ||
| break; | ||
| #ifdef __EMSCRIPTEN__ | ||
| emscripten_sleep(1); | ||
| #endif |
There was a problem hiding this comment.
This is used in a few places, should we wrap this into a helper and put it into wgpu-wasm-utils.h or something? Then actually the emscripten include could go there as well.
There was a problem hiding this comment.
good stuff. i managed to unify the logic into a utils function that should be also optimized for browsers as well. pushing soon
| @@ -1,29 +1,106 @@ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
Changes here look good. Thanks for moving towards generating the two sets of procs.
CMakeLists.txt
Outdated
| # Dawn WebGPU | ||
| if(NOT DEFINED SLANG_RHI_DAWN_URL) | ||
| # Dawn WebGPU (not used for Emscripten - uses emdawnwebgpu port instead) | ||
| if(NOT DEFINED SLANG_RHI_DAWN_URL AND NOT CMAKE_SYSTEM_NAME STREQUAL "Emscripten") |
There was a problem hiding this comment.
Should this also use AND NOT EMSCRIPTEN?
CMakeLists.txt
Outdated
| elseif(SLANG_RHI_ENABLE_WGPU AND EMSCRIPTEN) | ||
| # For Emscripten, WebGPU headers are provided by emdawnwebgpu port | ||
| # Add compile option to use the port which provides include paths | ||
| target_compile_options(slang-rhi PRIVATE "--use-port=emdawnwebgpu") | ||
| endif() |
There was a problem hiding this comment.
I would just move this to a second if block commented with # Link with Emscripten WebGPU. The elseif doesn't really make sense with the # Fetch & setup Dawn WebGPU
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Emscripten/WASM build and runtime support: CI workflow and CMake presets for Emscripten, Slang static/WASM library handling, tools to extract Emscripten WebGPU procs, new WASM-targeted example and HTML, and extensive WASM-specific codepaths across the WGPU layer (static proc linkage, polling waits, canvas surface, and buffer APIs). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Init as DeviceImpl::initCommon()
participant Inst as WGPU Instance
participant Adpt as AdapterRequestState
participant Dev as DeviceRequestState
participant EM as Emscripten Event Loop
App->>Init: initCommon()
Init->>Inst: wgpuInstanceRequestAdapter(callback)
Inst-->>Adpt: callback scheduled
Note over Init,EM: WASM polling loop (wgpu::wait)
loop poll until adapter ready
Init->>EM: emscripten_sleep(...)
EM->>Inst: process events
Inst-->>Adpt: update state
Init->>Init: check state.isComplete
end
Adpt-->>Init: adapter provided
Init->>Inst: wgpuAdapterRequestDevice(callback)
Inst-->>Dev: callback scheduled
Note over Init,EM: WASM polling loop (wgpu::wait)
loop poll until device ready
Init->>EM: emscripten_sleep(...)
EM->>Inst: process events
Inst-->>Dev: update state
Init->>Init: check state.isComplete
end
Dev-->>Init: device provided
Init->>App: return SLANG_OK
sequenceDiagram
autonumber
participant Caller as mapBuffer()
participant WGPU as WGPU Buffer
participant EM as Emscripten Event Loop
Caller->>WGPU: request map (wgpuBufferMapAsync)
alt SLANG_WASM
loop until mapped
Caller->>WGPU: poll status (wgpu::wait)
WGPU-->>Caller: not ready
Caller->>EM: emscripten_sleep(1)
EM->>WGPU: process events
end
else Non-WASM
Caller->>WGPU: wgpuInstanceWaitAny() (blocking)
WGPU-->>Caller: ready
end
Caller->>Caller: return mapped data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/platform.cpp (1)
5-71:⚠️ Potential issue | 🔴 CriticalAdd _GNU_SOURCE to Emscripten build configuration or guard dladdr for SLANG_WASM.
The code unconditionally uses
dladdr()andDl_infofor SLANG_WASM, but Emscripten's dlfcn.h only declares these when_GNU_SOURCEor_BSD_SOURCEis defined. This will cause a compilation error. Either enable_GNU_SOURCEin the CMakeLists.txt for Emscripten builds, or conditionally stubfindSharedLibraryPath()for SLANG_WASM since dynamic symbol lookup has limited practical use in WebAssembly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/platform.cpp` around lines 5 - 71, The SLANG_WASM build fails because findSharedLibraryPath() uses dladdr() and Dl_info which Emscripten only exposes when _GNU_SOURCE/_BSD_SOURCE is defined; fix by either (A) enabling _GNU_SOURCE for Emscripten targets in your build (CMake) so dladdr/Dl_info are declared, or (B) guard the wasm case in findSharedLibraryPath() (and related uses of dladdr/Dl_info) with SLANG_WASM and provide a safe stub that returns nullptr (or an appropriate fallback) since dynamic symbol lookup is limited in WebAssembly; update the implementation around findSharedLibraryPath, the SLANG_WASM preprocessor branches, and any callers that assume a non-null return.
🧹 Nitpick comments (2)
src/heap.cpp (1)
51-51: Consider using C++ style cast for consistency.The explicit cast to
uintptr_tis appropriate for WASM/Emscripten compatibility. However, line 93 usesstatic_cast<Page*>(), so using C++ style casts here would be more consistent throughout the file.♻️ Suggested change for consistency
- *outAllocation = {offset, size, page, pageAllocation.metadata, (uintptr_t)page->offsetToAddress(offset)}; + *outAllocation = {offset, size, page, pageAllocation.metadata, reinterpret_cast<uintptr_t>(page->offsetToAddress(offset))};- *outAllocation = {offset, size, newPage, pageAllocation.metadata, (uintptr_t)newPage->offsetToAddress(offset)}; + *outAllocation = {offset, size, newPage, pageAllocation.metadata, reinterpret_cast<uintptr_t>(newPage->offsetToAddress(offset))};Also applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/heap.cpp` at line 51, Replace the C-style cast used when constructing outAllocation so it uses a C++ style cast: change the (uintptr_t)page->offsetToAddress(offset) expression to static_cast<uintptr_t>(page->offsetToAddress(offset)), and make the same change for the other occurrence referenced (the one around the code that uses Page and offsetToAddress); locate the allocation construction where outAllocation is assigned and update the cast to static_cast<uintptr_t> to match the existing use of static_cast<Page*> and keep casting consistent across this file.src/wgpu/wgpu-device.cpp (1)
65-136: Applym_prefix to request‑state members.Rename the struct fields (e.g.,
status,adapter,device,message) tom_status,m_adapter,m_device,m_messageand update their uses for consistency.As per coding guidelines, "Member variables should start with 'm_' prefix and be in camelCase".
Also applies to: 229-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wgpu/wgpu-device.cpp` around lines 65 - 136, Rename AdapterRequestState's member fields to use the m_ prefix (e.g., status -> m_status, adapter -> m_adapter) and update all references accordingly: set requestState->m_status and requestState->m_adapter inside the WGPURequestAdapterCallback, check state->m_status in both the WASM and non‑WASM wait paths, and read state->m_adapter when assigning resultAdapter; also update the delete/state/null checks to use the new names. Apply the same m_ prefix renaming and fixups for the other request-state structs referenced elsewhere (e.g., fields named device, message) so all member accesses match the new m_<name> identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wgpu/wgpu-buffer.cpp`:
- Around line 124-138: In DeviceImpl::createBufferFromNativeHandle, validate the
native handle value before using it: if handle.type !=
NativeHandleType::WGPUBuffer or handle.value == 0, set *outBuffer = nullptr and
return SLANG_E_INVALID_HANDLE (avoid calling m_ctx.api.wgpuBufferAddRef on a
zero handle). When successful, proceed to create BufferImpl, assign
buffer->m_buffer from handle.value, call m_ctx.api.wgpuBufferAddRef, and return
via returnComPtr(outBuffer, buffer); ensure every early error path clears the
output pointer to nullptr to match the established pattern.
In `@tools/extract-webgpu-procs.py`:
- Around line 71-78: The except block for fetching EMSCRIPTEN_HEADER_URL only
initializes em_procs, leaving em_items undefined and causing UnboundLocalError;
modify the exception handling in the try/except around
fetch_url(EMSCRIPTEN_HEADER_URL) / extract_procs_from_content to either
initialize em_items (e.g., em_items = [] or set()) along with em_procs, or
return/exit early after logging the error so later code that references em_items
doesn't run; update the block that catches Exception as e to ensure both
em_items and em_procs are defined (or short-circuit) so subsequent references
are safe.
- Around line 14-49: The fallback branch in extract_procs_from_content returns
an empty items list when BEGIN_MARKER is missing, causing later macro emission
to have no proc entries; change the begin==-1 branch to build and return both
items (list of ("proc", name) tuples) and proc_names by iterating the typedef
regex matches (the re.finditer(r"typedef\s+\S+\s+\(\*WGPUProc(\w+)\)",
content)), appending ("proc", match.group(1)) to items and adding to proc_names
so downstream macro generation has valid proc entries.
---
Outside diff comments:
In `@src/core/platform.cpp`:
- Around line 5-71: The SLANG_WASM build fails because findSharedLibraryPath()
uses dladdr() and Dl_info which Emscripten only exposes when
_GNU_SOURCE/_BSD_SOURCE is defined; fix by either (A) enabling _GNU_SOURCE for
Emscripten targets in your build (CMake) so dladdr/Dl_info are declared, or (B)
guard the wasm case in findSharedLibraryPath() (and related uses of
dladdr/Dl_info) with SLANG_WASM and provide a safe stub that returns nullptr (or
an appropriate fallback) since dynamic symbol lookup is limited in WebAssembly;
update the implementation around findSharedLibraryPath, the SLANG_WASM
preprocessor branches, and any callers that assume a non-null return.
---
Nitpick comments:
In `@src/heap.cpp`:
- Line 51: Replace the C-style cast used when constructing outAllocation so it
uses a C++ style cast: change the (uintptr_t)page->offsetToAddress(offset)
expression to static_cast<uintptr_t>(page->offsetToAddress(offset)), and make
the same change for the other occurrence referenced (the one around the code
that uses Page and offsetToAddress); locate the allocation construction where
outAllocation is assigned and update the cast to static_cast<uintptr_t> to match
the existing use of static_cast<Page*> and keep casting consistent across this
file.
In `@src/wgpu/wgpu-device.cpp`:
- Around line 65-136: Rename AdapterRequestState's member fields to use the m_
prefix (e.g., status -> m_status, adapter -> m_adapter) and update all
references accordingly: set requestState->m_status and requestState->m_adapter
inside the WGPURequestAdapterCallback, check state->m_status in both the WASM
and non‑WASM wait paths, and read state->m_adapter when assigning resultAdapter;
also update the delete/state/null checks to use the new names. Apply the same m_
prefix renaming and fixups for the other request-state structs referenced
elsewhere (e.g., fields named device, message) so all member accesses match the
new m_<name> identifiers.
| def extract_procs_from_content(content: str) -> tuple[list[tuple[str, str]], set[str]]: | ||
| """ | ||
| Extract procs from header content. | ||
| Returns: | ||
| - List of (type, name) tuples where type is 'comment' or 'proc' | ||
| - Set of proc names | ||
| """ | ||
| begin = content.find(BEGIN_MARKER) | ||
| if begin == -1: | ||
| # Emscripten header uses different format - search for typedef patterns | ||
| procs = set() | ||
| for match in re.finditer(r"typedef\s+\S+\s+\(\*WGPUProc(\w+)\)", content): | ||
| procs.add(match.group(1)) | ||
| return [], procs | ||
|
|
||
| begin += len(BEGIN_MARKER) | ||
| end = content.find(END_MARKER) | ||
| lines = content[begin:end].splitlines() | ||
|
|
||
| items = [] | ||
| proc_names = set() | ||
|
|
||
| for line in lines: | ||
| line = line.strip() | ||
| if line == "": | ||
| continue | ||
| name = RE_PROC.search(line).group(1) | ||
| output += " x(" + name + ") \\\n" | ||
| if line.startswith("//"): | ||
| items.append(("comment", line[3:])) | ||
| else: | ||
| match = RE_PROC.search(line) | ||
| if match: | ||
| name = match.group(1) | ||
| items.append(("proc", name)) | ||
| proc_names.add(name) | ||
|
|
||
| return items, proc_names |
There was a problem hiding this comment.
Fallback extraction drops proc items, producing invalid macro output.
When BEGIN_MARKER is missing you return an empty items list, but later emit the macro from em_items. That yields a dangling backslash and no procs. Populate items from the typedef matches so output remains valid.
🐛 Proposed fix
- if begin == -1:
- # Emscripten header uses different format - search for typedef patterns
- procs = set()
- for match in re.finditer(r"typedef\s+\S+\s+\(\*WGPUProc(\w+)\)", content):
- procs.add(match.group(1))
- return [], procs
+ if begin == -1:
+ # Emscripten header uses different format - search for typedef patterns
+ items = []
+ procs = set()
+ for match in re.finditer(r"typedef\s+\S+\s+\(\*WGPUProc(\w+)\)", content):
+ name = match.group(1)
+ if name in procs:
+ continue
+ items.append(("proc", name))
+ procs.add(name)
+ return items, procs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/extract-webgpu-procs.py` around lines 14 - 49, The fallback branch in
extract_procs_from_content returns an empty items list when BEGIN_MARKER is
missing, causing later macro emission to have no proc entries; change the
begin==-1 branch to build and return both items (list of ("proc", name) tuples)
and proc_names by iterating the typedef regex matches (the
re.finditer(r"typedef\s+\S+\s+\(\*WGPUProc(\w+)\)", content)), appending
("proc", match.group(1)) to items and adding to proc_names so downstream macro
generation has valid proc entries.
| print(f"// Fetching Emscripten header from {EMSCRIPTEN_HEADER_URL}...") | ||
| try: | ||
| em_content = fetch_url(EMSCRIPTEN_HEADER_URL) | ||
| em_items, em_procs = extract_procs_from_content(em_content) | ||
| except Exception as e: | ||
| print(f"// Error fetching Emscripten header: {e}") | ||
| em_procs = set() | ||
|
|
There was a problem hiding this comment.
em_items is undefined on fetch failure.
The exception path only sets em_procs, but em_items is referenced later, causing an UnboundLocalError. Initialize it or return early.
🐛 Proposed fix
- except Exception as e:
- print(f"// Error fetching Emscripten header: {e}")
- em_procs = set()
+ except Exception as e:
+ print(f"// Error fetching Emscripten header: {e}")
+ em_items = []
+ em_procs = set()🧰 Tools
🪛 Ruff (0.15.1)
[warning] 75-75: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/extract-webgpu-procs.py` around lines 71 - 78, The except block for
fetching EMSCRIPTEN_HEADER_URL only initializes em_procs, leaving em_items
undefined and causing UnboundLocalError; modify the exception handling in the
try/except around fetch_url(EMSCRIPTEN_HEADER_URL) / extract_procs_from_content
to either initialize em_items (e.g., em_items = [] or set()) along with
em_procs, or return/exit early after logging the error so later code that
references em_items doesn't run; update the block that catches Exception as e to
ensure both em_items and em_procs are defined (or short-circuit) so subsequent
references are safe.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
CMakeLists.txt (1)
500-517: Consider restructuring WebGPU port handling into a separate block.A past review suggested using a separate
ifblock with a distinct comment for the Emscripten WebGPU port, rather than anelseifattached to the Dawn fetch block. This would improve clarity since the emdawnwebgpu port is conceptually distinct from the Dawn binary fetch.Suggested restructure
# Fetch & setup Dawn WebGPU if(SLANG_RHI_ENABLE_WGPU AND NOT EMSCRIPTEN) message(STATUS "Fetching Dawn WebGPU ...") FetchPackage(dawn URL ${SLANG_RHI_DAWN_URL}) add_library(slang-rhi-dawn INTERFACE) target_include_directories(slang-rhi-dawn INTERFACE ${dawn_SOURCE_DIR}/include) if(CMAKE_SYSTEM_NAME STREQUAL "Windows") copy_file(${dawn_SOURCE_DIR}/bin/dawn.dll .) elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") copy_file(${dawn_SOURCE_DIR}/lib64/libdawn.so .) elseif(CMAKE_SYSTEM_NAME STREQUAL "Darwin") copy_file(${dawn_SOURCE_DIR}/lib/libdawn.dylib .) endif() target_link_libraries(slang-rhi PRIVATE slang-rhi-dawn) -elseif(SLANG_RHI_ENABLE_WGPU AND EMSCRIPTEN) +endif() + +# Link with Emscripten WebGPU +if(SLANG_RHI_ENABLE_WGPU AND EMSCRIPTEN) # For Emscripten, WebGPU headers are provided by emdawnwebgpu port # Add compile option to use the port which provides include paths target_compile_options(slang-rhi PRIVATE "--use-port=emdawnwebgpu") endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 500 - 517, The WebGPU Emscripten handling should be split out from the Dawn fetch elseif into its own clearly commented if-block: when SLANG_RHI_ENABLE_WGPU is true and EMSCRIPTEN is set, add the compile option "--use-port=emdawnwebgpu" for target_compile_options(slang-rhi) in a standalone if(SLANG_RHI_ENABLE_WGPU AND EMSCRIPTEN) block with a comment explaining the emscripten port, and keep the FetchPackage(dawn)/target_include_directories(slang-rhi-dawn)/copy_file logic inside the separate if(SLANG_RHI_ENABLE_WGPU AND NOT EMSCRIPTEN) block so the two flows are conceptually and visually distinct.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
275-282: Consider usingWARNINGinstead ofSTATUSfor version override.When the Slang version is overridden due to missing WASM support, a
WARNINGlevel message would be more visible to users who may have explicitly set an older version.Suggestion
if(SLANG_VERSION VERSION_LESS SLANG_WASM_MIN_VERSION) - message(STATUS "Slang ${SLANG_VERSION} did not release WASM libraries, overriding to ${SLANG_WASM_MIN_VERSION}") + message(WARNING "Slang ${SLANG_VERSION} did not release WASM libraries, overriding to ${SLANG_WASM_MIN_VERSION}") set(SLANG_VERSION ${SLANG_WASM_MIN_VERSION}) endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 275 - 282, Change the message() level from STATUS to WARNING in the Slang WASM override block: when CMAKE_SYSTEM_NAME is "Emscripten" and SLANG_VERSION is forced to SLANG_WASM_MIN_VERSION, call message(WARNING ...) instead of message(STATUS ...) so the override of SLANG_VERSION and the value of SLANG_WASM_MIN_VERSION are more visible to users; update the existing message invocation that references SLANG_VERSION and SLANG_WASM_MIN_VERSION accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 500-517: The WebGPU Emscripten handling should be split out from
the Dawn fetch elseif into its own clearly commented if-block: when
SLANG_RHI_ENABLE_WGPU is true and EMSCRIPTEN is set, add the compile option
"--use-port=emdawnwebgpu" for target_compile_options(slang-rhi) in a standalone
if(SLANG_RHI_ENABLE_WGPU AND EMSCRIPTEN) block with a comment explaining the
emscripten port, and keep the
FetchPackage(dawn)/target_include_directories(slang-rhi-dawn)/copy_file logic
inside the separate if(SLANG_RHI_ENABLE_WGPU AND NOT EMSCRIPTEN) block so the
two flows are conceptually and visually distinct.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 275-282: Change the message() level from STATUS to WARNING in the
Slang WASM override block: when CMAKE_SYSTEM_NAME is "Emscripten" and
SLANG_VERSION is forced to SLANG_WASM_MIN_VERSION, call message(WARNING ...)
instead of message(STATUS ...) so the override of SLANG_VERSION and the value of
SLANG_WASM_MIN_VERSION are more visible to users; update the existing message
invocation that references SLANG_VERSION and SLANG_WASM_MIN_VERSION accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci.ymlCMakeLists.txtinclude/slang-rhi.hsrc/heap.cppsrc/rhi.cppsrc/wgpu/wgpu-command.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- src/wgpu/wgpu-command.cpp
|
@j8asic Looks like there are bunch of compilation failures. Can you also look at the review comments from coderabbit? I'll take another look when you feel it's ready. |
yeah I had to stop in the middle of pushing and now resumed based on your comments sorry |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/wasm/index.html`:
- Around line 1-25: The HTML is missing a language declaration and the live log
element isn't exposed to assistive tech; update the <html> element to include a
lang attribute (e.g., lang="en") and make the <pre id="output"> a live region by
adding ARIA attributes such as role="status" and aria-live="polite" (optionally
aria-atomic="true") so output appended by Module.print / Module.printErr is
announced to screen readers.
skallweitNV
left a comment
skallweitNV
left a comment
There was a problem hiding this comment.
Mostly looks good now.
CMakeLists.txt
Outdated
| if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten") | ||
| # Emscripten WASM releases are available starting from v2026.3.1 | ||
| set(SLANG_WASM_MIN_VERSION "2026.3.1") | ||
| if(SLANG_VERSION VERSION_LESS SLANG_WASM_MIN_VERSION) | ||
| message(STATUS "Slang ${SLANG_VERSION} did not release WASM libraries, overriding to ${SLANG_WASM_MIN_VERSION}") | ||
| set(SLANG_VERSION ${SLANG_WASM_MIN_VERSION}) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Can we remove this? slang-rhi is now upated to use 2026.3.1 so this should be obsolete.
There was a problem hiding this comment.
ok that is good news, this was temp till rhi gets updated
CMakeLists.txt
Outdated
| INTERFACE_INCLUDE_DIRECTORIES ${SLANG_RHI_SLANG_INCLUDE_DIR} | ||
| IMPORTED_LOCATION ${SLANG_RHI_SLANG_BINARY_DIR}/lib/lib${SLANG_LIB_NAME}.dylib | ||
| ) | ||
| elseif(EMSCRIPTEN) |
There was a problem hiding this comment.
can we use elseif(CMAKE_SYSTEM_NAME STREQUAL "Emscripten") here for consistency?
| if(NOT EMSCRIPTEN) | ||
| function(add_example name source) | ||
| add_executable(${name} ${source}) | ||
| target_compile_features(${name} PRIVATE cxx_std_20) | ||
| get_filename_component(EXAMPLE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/${source}" PATH) | ||
| target_compile_definitions(${name} PRIVATE SLANG_RHI_DEBUG=$<BOOL:$<CONFIG:Debug>> EXAMPLE_DIR="${EXAMPLE_DIR}" NOMINMAX) | ||
| target_include_directories(${name} PRIVATE examples/base) | ||
| target_link_libraries(${name} PRIVATE slang-rhi slang glfw) | ||
| endfunction() | ||
|
|
||
| add_example(example-shader-toy examples/shader-toy/example-shader-toy.cpp) | ||
| add_example(example-surface examples/surface/example-surface.cpp) | ||
| add_example(example-triangle examples/triangle/example-triangle.cpp) | ||
|
|
||
| add_example(example-shader-toy examples/shader-toy/example-shader-toy.cpp) | ||
| add_example(example-surface examples/surface/example-surface.cpp) | ||
| add_example(example-triangle examples/triangle/example-triangle.cpp) | ||
| else() | ||
| add_executable(wasm-test examples/wasm/wasm-test.cpp) | ||
| target_compile_features(wasm-test PRIVATE cxx_std_17) | ||
| target_link_libraries(wasm-test PRIVATE slang-rhi slang) | ||
| set_target_properties(wasm-test PROPERTIES | ||
| OUTPUT_NAME "wasm-test" | ||
| SUFFIX ".html" | ||
| ) | ||
| target_link_options(wasm-test PRIVATE | ||
| "--use-port=emdawnwebgpu" | ||
| "--shell-file" "${CMAKE_CURRENT_SOURCE_DIR}/examples/wasm/index.html" | ||
| ) |
There was a problem hiding this comment.
Not super happy about this .. I think the goal should be that examples generally also work on WASM. But we can leave this for now.
There was a problem hiding this comment.
i would rather try to make another PR for testing (iirc you wrote you need to think how to handle automatic testing?)
There was a problem hiding this comment.
yes, this is fine for now
CMakePresets.json
Outdated
| "CMAKE_C_FLAGS_INIT": "-fwasm-exceptions -Os", | ||
| "CMAKE_CXX_FLAGS_INIT": "-fwasm-exceptions -Os", |
There was a problem hiding this comment.
Do we want -Os size optimizations?
CMakePresets.json
Outdated
| "SLANG_RHI_ENABLE_CPU": "OFF", | ||
| "CMAKE_C_FLAGS_INIT": "-fwasm-exceptions -Os", | ||
| "CMAKE_CXX_FLAGS_INIT": "-fwasm-exceptions -Os", | ||
| "CMAKE_EXE_LINKER_FLAGS": "-sASSERTIONS -sALLOW_MEMORY_GROWTH -fwasm-exceptions --export=__cpp_exception -sASYNCIFY -sASYNCIFY_IGNORE_INDIRECT=1 -sASYNCIFY_ONLY=['*main*','*wait*','*initialize*','*map*','*readBuffer*','*createBuffer*']" |
There was a problem hiding this comment.
What does ASYNCIFY do? Feels bad to have to add this to the linker flags ...
There was a problem hiding this comment.
this is the price for using sync functions without freezing the browser, and this is the best outcome i.e. to tag only specific functions needing to use asyncify. Otherwise the file size would be large, and compilation is slow
| .DS_Store | ||
| .vscode/ | ||
| build/ | ||
| build.em/ |
There was a problem hiding this comment.
Why does emscripten build not use build folder?
There was a problem hiding this comment.
TBH don't know, you tell me (taken this practice from the Slang repo)
There was a problem hiding this comment.
is our build ending up in build.em at all?
There was a problem hiding this comment.
yes "binaryDir": "${sourceDir}/build.em",
src/wgpu/wgpu-utils.h
Outdated
| struct Context; | ||
| typedef Context WGPUContext; |
There was a problem hiding this comment.
Can't we just use Context without introducing a type alias?
src/wgpu/wgpu-buffer.cpp
Outdated
|
|
||
| #include "core/deferred.h" | ||
|
|
||
|
|
src/wgpu/wgpu-command.cpp
Outdated
| #include "wgpu-shader-object.h" | ||
| #include "wgpu-utils.h" | ||
|
|
||
|
|
src/wgpu/wgpu-device.cpp
Outdated
| WGPURequestAdapterStatus status = WGPURequestAdapterStatus(0); | ||
| WGPUAdapter adapter = nullptr; | ||
| }; | ||
| AdapterRequestState* state = new AdapterRequestState(); |
There was a problem hiding this comment.
Does this need to be heap allocated?
There was a problem hiding this comment.
I've read about the browser caveats, and went with safer option not to trust asyncify stack saving. But maybe newer emscripten and browsers have it under control. Let me test the stack version and then update
|
@j8asic sorry for not being responsive on this, I've been busy. Is this ready for a "final" review? :D |
|
@skallweitNV no worries I have been busy as well. The code is used already in prod from my side (along with sgl in wasm) |
skallweitNV
left a comment
skallweitNV
left a comment
There was a problem hiding this comment.
LGTM, let's update this to latest main and then we can merge.
| buffer->m_buffer = m_ctx.api.wgpuDeviceCreateBuffer(m_ctx.device, &bufferDesc); | ||
| if (!buffer->m_buffer) | ||
| { | ||
| *outBuffer = nullptr; |
There was a problem hiding this comment.
I think client code should not rely on this. Is this needed?
| ); | ||
| #else | ||
| SLANG_UNUSED(cmd); | ||
| NOT_SUPPORTED(S_RenderPassEncoder_drawIndirect); |
There was a problem hiding this comment.
this will need to be changed to NOT_SUPPORTED(IRenderPassEncoder, drawIndirect); when updating to latest main.
| ); | ||
| #else | ||
| SLANG_UNUSED(cmd); | ||
| NOT_SUPPORTED(S_RenderPassEncoder_drawIndexedIndirect); |
There was a problem hiding this comment.
NOT_SUPPORTED(IRenderPassEncoder, drawIndexedIndirect);
|
|
||
| #include "core/assert.h" | ||
|
|
||
| #include "wgpu-device.h" |
There was a problem hiding this comment.
wgpu includes should come first, move this before or after wgpu-utils.h
3 participants
Added support for compiling
slang-rhiusing Emscripten (WASM), while enabling the WebGPU backend and handling dependencies for the web platform:SLANG_RHI_HAS_WGPUand disable incompatible native APIs when building for Emscripten--use-port=emdawnwebgpufor Emscripten buildsSummary by CodeRabbit
New Features
Compatibility
Chores
Bug Fixes