Skip to content

Convert gfx unit tests and examples to use slang-rhi #7577

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 26 commits into from
Jul 9, 2025
Merged

Conversation

gtong-nv
Copy link
Contributor

@gtong-nv gtong-nv commented Jul 1, 2025

  • All examples and gfx-unit-tests are ported to use slang-rhi and PASS in CI
  • Some tests are already ported and running in slang-rhi CI; can be remove to save CI capacity
  • Some tests are disabled for now given no equivalent API in slang-rhi and no longer applicable
  • See summary below

Disabled/Problematic Tests

Tests that need attention or have unresolved issues:

Test File Issue Action Needed
precompiled-module-2.cpp Test failed - segfault porting WIP in slang-rhi\tests\test-precompiled-module-cache.cpp
root-mutable-shader-object.cpp Test failed - buffer not being modified by shader porting WIP in slang-rhi\tests\test-mutable-shader-object.cpp
get-supported-resource-states-test.cpp No equivalent API; Replaced by IDevice::getFormatSupport Covered by other test; Can remove
resolve-resource-tests.cpp No resolveResource API Covered by other test; Can remove

Duplicated Tests (Ready for Removal)

These tests have equivalent implementations in slang-rhi and can be safely deleted:

GFX Test File Slang-RHI Equivalent Status
clear-texture-test.cpp test-cmd-clear-texture.cpp ✅ Can remove
copy-texture-tests.cpp test-cmd-copy-texture.cpp ✅ Can remove
format-unit-tests.cpp test-formats.cpp ✅ Can remove
instanced-draw-tests.cpp test-cmd-draw.cpp ✅ Can remove
shader-cache-tests.cpp test-shader-cache.cpp ✅ Can remove
shared-textures-tests.cpp test-texture-shared.cpp ✅ Can remove
swap-chain-resize-test.cpp test-surface.cpp ✅ Can remove
texture-types-tests.cpp test-texture-types.cpp ✅ Can remove
sampler-array.cpp test-sampler-array.cpp ✅ Fixed & can remove
uint16-structured-buffer.cpp test-uint16-buffer.cpp ✅ Fixed & can remove
root-shader-parameter.cpp test-root-shader-parameter.cpp ✅ Fixed & can remove

Closes: #7564

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2025

CLA assistant check
All committers have signed the CLA.

@gtong-nv gtong-nv force-pushed the gfx-to-slang-rhi branch from aa5aff0 to 40e5f1c Compare July 3, 2025 17:20
@gtong-nv
Copy link
Contributor Author

gtong-nv commented Jul 5, 2025

/format

@slangbot
Copy link
Contributor

slangbot commented Jul 5, 2025

🌈 Formatted, please merge the changes from this PR

@gtong-nv gtong-nv marked this pull request as ready for review July 7, 2025 17:41
@gtong-nv gtong-nv requested a review from a team as a code owner July 7, 2025 17:41
@csyonghe
Copy link
Collaborator

csyonghe commented Jul 8, 2025

There is also an tests/cpu-program/gfx-smoke.slang test that uses gfx, we need to move it to rhi too. But this will require us do a slang-rhi.slang to expose the rhi interface to slang.

@gtong-nv gtong-nv force-pushed the gfx-to-slang-rhi branch from d150eed to cfbc002 Compare July 8, 2025 06:15
@gtong-nv
Copy link
Contributor Author

gtong-nv commented Jul 8, 2025

There is also an tests/cpu-program/gfx-smoke.slang test that uses gfx, we need to move it to rhi too. But this will require us do a slang-rhi.slang to expose the rhi interface to slang.

I removed tests/cpu-program/gfx-smoke.slang for now thinking it was used to test the slang-gfx layer. Do you mean we should move tools\gfx\gfx.slang to an slang-rhi.slang module in slang-rhi?

@gtong-nv gtong-nv added the pr: non-breaking PRs without breaking changes label Jul 8, 2025
@gtong-nv
Copy link
Contributor Author

gtong-nv commented Jul 8, 2025

/format

@slangbot
Copy link
Contributor

slangbot commented Jul 8, 2025

🌈 Formatted, please merge the changes from this PR

Co-authored-by: slangbot <[email protected]>
jkwak-work
jkwak-work previously approved these changes Jul 8, 2025
Copy link
Collaborator

@jkwak-work jkwak-work left a comment

Choose a reason for hiding this comment

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

Great job!
Looks good to me.

Comment on lines +2 to +5
* This test has been disabled because the slang-rhi API
* does not provide equivalent functionality for querying format-supported
* resource states. The old gfx API's getFormatSupportedResourceStates() is
* replaced by with IDevice::getFormatSupport.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@skallweitNV , do we have a plan to support the feature on RHI?
If not, we should remove this file entirely.

Comment on lines +113 to +115
// For now, skip pipeline creation since we don't have a shader program
// This would need to be completed for full functionality
#if 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need an issue to follow up this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create an issue

@gtong-nv gtong-nv force-pushed the gfx-to-slang-rhi branch from e5ac107 to 3ded299 Compare July 8, 2025 21:37
@gtong-nv gtong-nv enabled auto-merge July 8, 2025 21:39
@kaizhangNV
Copy link
Contributor

There is also an tests/cpu-program/gfx-smoke.slang test that uses gfx, we need to move it to rhi too. But this will require us do a slang-rhi.slang to expose the rhi interface to slang.

I removed tests/cpu-program/gfx-smoke.slang for now thinking it was used to test the slang-gfx layer. Do you mean we should move tools\gfx\gfx.slang to an slang-rhi.slang module in slang-rhi?

gfx-smoke.slang is slang code that will targeting CPU, and that code is to use gfx to run a simple slang compute shader. It's to demonstrate the CPU target is working, and our implementation for gfx.slang and slang.slang is working correctly.

gfx.slang and slang.slang are located at tools/gfx/ directory, you can see they are slang modules imported at beginning of gfx-smoke.slang.

slang.slang and gfx.slang includes minimum required definitions that is good enough for gfx-smoke.slang to run.

slang.slang are just copying from slang.h
and gfx.slang are just copying from gfx.h, but since those are slang code, so some syntax has to be changed.

To convert gfx-smoke.slang to rhi-smoke.slang, you need to not only change this file, but also convert gfx.slang to rhi.slang to make thing consistent both.

But most changes are just trivial, because gfx and rhi shares lots of definitions.

Copy link
Collaborator

@jkwak-work jkwak-work left a comment

Choose a reason for hiding this comment

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

Looks good to me

But I am not sure what is going on with rhi.slang.

Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@gtong-nv gtong-nv added this pull request to the merge queue Jul 9, 2025
Merged via the queue into master with commit 43d0c21 Jul 9, 2025
25 of 26 checks passed
@@ -139,119 +139,47 @@ if(SLANG_ENABLE_GFX)
imgui
stb
$<$<BOOL:${SLANG_ENABLE_XLIB}>:X11::X11>
$<$<BOOL:${SLANG_ENABLE_SLANG_RHI}>:slang-rhi>
Copy link

@niklaskorz niklaskorz Jul 30, 2025

Choose a reason for hiding this comment

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

This suggests that enabling slang-rhi to compile the platform module is optional.
However, when setting SLANG_ENABLE_SLANG_RHI=OFF, compilation fails as tools/platfom/gui.h tries to include <slang-rhi.h> regardless of this option.

Choose a reason for hiding this comment

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

We currently rely on SLANG_ENABLE_SLANG_RHI=OFF in nixpkgs as slang-rhi tries to download all of its dependencies through FetchPackage and there is no way to provide them offline it seems, thus we have to keep slang-rhi disabled in nixpkgs.

This was referenced Aug 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert gfx to slang-rhi for gfx-unit-test
7 participants