Enable Metal in pointer test suite#958
Conversation
Mirrors the existing SGL_LOCAL_SLANG option. When SGL_LOCAL_RHI=ON, slang-rhi is built from SGL_LOCAL_RHI_DIR instead of the submodule. Made-with: Cursor
Add DeviceType.metal to POINTER_DEVICE_TYPES so pointer tests run on Metal hardware. This exercises the slang-rhi argument buffer pointer data path that was previously broken (see shader-slang/slang#10841). Made-with: Cursor
The `&` operator is intended to be removed soon, no point enabling this for macos.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR extends documentation for local slang-rhi build configuration and implements configurable CMake support for building against a local slang-rhi source tree with conditional fallback logic. Additionally, pointer tests are parameterized to include Metal device type with selective device-specific skips. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
slangpy/tests/slangpy_tests/test_pointers.py (1)
594-596: Skip looks correct; considerpytest.mark.skipiffor clarity.The runtime skip is fine and matches the shader on Line 609 which uses
&buffer[call_id%10].value. As an optional refactor,@pytest.mark.skipif(... , reason=...)keeps the gating with the parametrize decorators rather than at the top of the body, but the current form is consistent with other conditional skips in the file (e.g. Line 768).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slangpy/tests/slangpy_tests/test_pointers.py` around lines 594 - 596, The test test_atomic_float_in_struct_access currently performs a runtime skip for DeviceType.metal; replace this with a pytest.mark.skipif decorator on the test to make the gating declarative — use pytest.mark.skipif(device_type == DeviceType.metal, reason="operator& is not supported on Metal and is being removed from Slang") so the skip is applied at collection time and sits alongside the parametrize decorators rather than inside the test body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@slangpy/tests/slangpy_tests/test_pointers.py`:
- Around line 594-596: The test test_atomic_float_in_struct_access currently
performs a runtime skip for DeviceType.metal; replace this with a
pytest.mark.skipif decorator on the test to make the gating declarative — use
pytest.mark.skipif(device_type == DeviceType.metal, reason="operator& is not
supported on Metal and is being removed from Slang") so the skip is applied at
collection time and sits alongside the parametrize decorators rather than inside
the test body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98031c6c-d460-452a-8a05-3264573b42b3
📒 Files selected for processing (3)
AGENTS.mdexternal/CMakeLists.txtslangpy/tests/slangpy_tests/test_pointers.py
Updates external/slang-rhi to a7e218ee which includes Metal residency tracking and GPU address support needed for pointer operations. Made-with: Cursor
Sync the Feature enum with slang-rhi after the addition of Feature::ResidencySet, fixing a static_assert size mismatch. Made-with: Cursor
The underlying crash ("invalid reflection data") appears to have been
fixed by upstream slang-rhi changes. Verified passing on both the
MTLResidencySet and useResources fallback paths.
Made-with: Cursor
| @@ -226,10 +229,16 @@ set(SLANG_RHI_BUILD_FROM_SLANG_REPO ON) | |||
| set(SLANG_RHI_FETCH_SLANG OFF) | |||
| set(SLANG_RHI_SLANG_INCLUDE_DIR ${SLANG_INCLUDE_DIR} CACHE STRING "" FORCE) | |||
| set(SLANG_RHI_SLANG_BINARY_DIR ${SLANG_DIR} CACHE STRING "" FORCE) | |||
| add_subdirectory(slang-rhi) | |||
| if(SGL_LOCAL_RHI) | |||
| add_subdirectory(${SGL_LOCAL_RHI_DIR} ${CMAKE_BINARY_DIR}/slang-rhi) | |||
| set(SGL_RHI_SOURCE_DIR ${SGL_LOCAL_RHI_DIR}) | |||
| else() | |||
| add_subdirectory(slang-rhi) | |||
| set(SGL_RHI_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/slang-rhi) | |||
| endif() | |||
There was a problem hiding this comment.
I would probably rename these to SGL_LOCAL_SLANG_RHI and SGL_LOCAL_SLANG_RHI_DIR to be more consistent with SGL_LOCAL_SLANG. Also rename SGL_RHI_SOURCE_DIR to SGL_SLANG_RHI_SOURCE_DIR
ccummingsNV
left a comment
ccummingsNV
left a comment
There was a problem hiding this comment.
Let's make the twaks Simon proposes, and need to look at fixing the tests
3 participants
Summary
SGL_LOCAL_RHIcmake option to build slang-rhi from a local checkout (mirrorsSGL_LOCAL_SLANG)DeviceType.metalinPOINTER_DEVICE_TYPESso pointer tests run on Metal hardware, exercising the slang-rhi argument buffer pointer data path&operator pointer tests on Metal (the&operator is being removed)Fixes shader-slang/slang#10841.
TODO:
external/slang-rhisubmodule pointer in this PR to include it.Test plan