Skip to content
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

WGSL: Aim for getting all GFX tests to pass #4943

Closed
aleino-nv opened this issue Aug 28, 2024 · 22 comments
Closed

WGSL: Aim for getting all GFX tests to pass #4943

aleino-nv opened this issue Aug 28, 2024 · 22 comments
Assignees
Labels
goal:forward looking Feature needed at a later date, not connected to a specific use case. kind:enhancement a desirable new feature, option, or behavior

Comments

@aleino-nv
Copy link
Collaborator

aleino-nv commented Aug 28, 2024

No description provided.

@aleino-nv aleino-nv self-assigned this Aug 28, 2024
@aleino-nv aleino-nv added goal:forward looking Feature needed at a later date, not connected to a specific use case. kind:enhancement a desirable new feature, option, or behavior labels Aug 28, 2024
@bmillsNV bmillsNV added this to the Q4 2024 (Fall) milestone Aug 29, 2024
@bmillsNV bmillsNV assigned jkwak-work and unassigned aleino-nv Oct 1, 2024
@jkwak-work
Copy link
Collaborator

jkwak-work commented Oct 2, 2024

The plan is to try to get tests/render/cross-compile-entry-point.slang working for WGSL and see what comes out next.

@jkwak-work
Copy link
Collaborator

jkwak-work commented Oct 3, 2024

After spending some time understanding how tests/render/cross-compile-entry-point.slang works, here is how I understood this issue.

There are four slang-test "test-types" related to "rendering"; the full list of "test-types" can be found here.:

  • COMPARE_HLSL_RENDER
  • COMPARE_HLSL_CROSS_COMPILE_RENDER
  • COMPARE_HLSL_GLSL_RENDER
  • COMPARE_RENDER_COMPUTE

Because this task is limited to "all GFX tests", "COMPARE_RENDER_COMPUTE" isn't relevant.

There are only 7 tests that use one of the three test-types:

tests/bugs/texture2d-gather.hlsl
tests/render/cross-compile-entry-point.slang // currently disabled
tests/render/nointerpolation.hlsl // currently disabled
tests/render/render0.hlsl // currently disabled
tests/render/cross-compile0.hlsl // currently disabled
tests/render/imported-parameters.hlsl // currently disabled
tests/render/unused-discard.hlsl // currently disabled

The goal of this task will be to make those tests working for WGSL.

For the disabled tests, we will need to re-evaluate if they should be re-enabled.
If enabled, they will also need to work for WGSL.
As an example, "tests/render/cross-compile-entry-point.slang" can be simply re-enabled by changing the test-type from COMPARE_HLSL_CROSS_COMPILE_RENDER to COMPARE_HLSL_RENDER.

When the task is completed, we should have a following line on those rendering tests,

//TEST:COMPARE_HLSL_RENDER:-wgpu

It will generate two PNG image files: one with HLSL/DXC and another with WGSL/DAWN.
slang-test will compare the images and check if they are identical.

@csyonghe
Copy link
Collaborator

csyonghe commented Oct 3, 2024

COMPARE_RENDER_COMPUTE is actually relevant, it is a vertex-fragment pipeline that writes results into a buffer instead of a framebuffer, and reads back the buffer values for comparison, so that test is a vertex-fragment shader test.

@jkwak-work
Copy link
Collaborator

I see.
Thanks for the information.

For COMPARE_RENDER_COMPUTE, there are six tests that use the test-type:

./compute/compile-time-loop.slang
./compute/discard-stmt.slang
./compute/global-type-param-in-entrypoint.slang // currently disabled
./compute/texture-sample-grad-offset-clamp.slang
./compute/texture-sampling.slang
./compute/type-param-varying.slang // currently disabled

@jkwak-work
Copy link
Collaborator

It appears that a big portion of the implementation for this task is already done in PR 5174.

@bmillsNV bmillsNV assigned aleino-nv and unassigned jkwak-work Oct 8, 2024
@bmillsNV
Copy link
Collaborator

bmillsNV commented Oct 8, 2024

Moving to Anders.

@aleino-nv
Copy link
Collaborator Author

aleino-nv commented Nov 12, 2024

I'll just start with compute/compile-time-loop.slang.

Here we end up with Slang-RHI/WGPU failing because we're trying to create a vertex buffer with MemoryType::Upload, and the Slang-RHI/WGPU implementation currently has a bug where it will violate WebGPU requirements if this request is made.

WGPU error: Buffer usages (BufferUsage::(MapWrite|CopySrc|CopyDst)) is invalid. If a buffer usage contains BufferUsage::MapWrite the only other allowed usage is BufferUsage::CopySrc.
 - While calling [Device].CreateBuffer([BufferDescriptor "Unnamed buffer (size=96, elementSize=0, format=Unknown, memoryType=Upload, usage=VertexBuffer, defaultState=VertexBuffer)"]).

@aleino-nv
Copy link
Collaborator Author

aleino-nv commented Nov 12, 2024

This is a known problem I've discussed with @skallweitNV previously.
shader-slang/slang-rhi#104

The issue is that MemoryType::Upload makes Slang-RHI try to create a host-mappable buffer, that can also be used in other ways.
This is not allowed in WebGPU: mappable buffers cannot have any other usages.

I'll discuss with @skallweitNV on what should be done.
I think Slang-RHI needs to change in some way as to accommodate this WebGPU limitation.

@aleino-nv
Copy link
Collaborator Author

This is a known problem I've discussed with @skallweitNV previously. shader-slang/slang-rhi#104

The issue is that MemoryType::Upload makes Slang-RHI try to create a host-mappable buffer, that can also be used in other ways. This is not allowed in WebGPU: mappable buffers cannot have any other usages.

I'll discuss with @skallweitNV on what should be done. I think Slang-RHI needs to change in some way as to accommodate this WebGPU limitation.

After fixing this by changing slang-test to use MemoryType::DeviceLocal, there is now a binding mismatch issue.
Slang-RHI is creating a pipeline with one bind group layout containing 3 bindings at indicies 0-2 that match the fragment shader, followed by one binding at index 3 that seems to match the vertex shader, except that the @binding attribute in the vertex shader is 0 instead of 3.

WGPU error: Binding type in the shader (buffer) doesn't match the type in the layout (texture).
 - While validating that the entry-point's declaration for @group(0) @binding(0) matches [BindGroupLayout (unlabeled)]
 - While validating the entry-point's compatibility for group 0 with [BindGroupLayout (unlabeled)]
 - While validating vertex stage ([ShaderModule (unlabeled)], entryPoint: vertexMain).
 - While validating vertex state.
 - While calling [Device].CreateRenderPipeline([RenderPipelineDescriptor]).

In the Vulkan case, the one binding for the vertex shader gets index 3, which matches the WGSL layout.

I suppose the error is on the Slang side: the vertex shader binding should get index 3 but gets index 0 for some reason.

aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 13, 2024
There was a "kinds" vs. "kind flags" mismatch, and also getBindingOffsetForKinds was not
being used.

This helps to address issue shader-slang#4943.
@aleino-nv
Copy link
Collaborator Author

#5548 fixes a WGSL emitter layout issue and enables compute/compile-time-loop.slang

Looking at some of the other tests now.

@aleino-nv
Copy link
Collaborator Author

compute/texture-sampling.slang fails due to #5223

@aleino-nv
Copy link
Collaborator Author

  • tests/bugs/texture2d-gather.hlsl: PASS
  • tests/compute/compile-time-loop.slang: PASS
  • tests/compute/discard-stmt.slang: PASS
  • tests/compute/global-type-param-in-entrypoint.slang: (Disabled for all back-ends?)
  • tests/compute/texture-sample-grad-offset-clamp.slang: (There is no SampleGrad that takes a lodClamp parameter in WGSL)
  • tests/compute/texture-sampling.slang: (There is no texture_1d_array in WGSL)
  • tests/compute/type-param-varying.slang: (Disabled for all back-ends?)
  • tests/render/cross-compile-entry-point.slang: (No tests defined for any back-ends?)
  • tests/render/cross-compile0.hlsl: (Disabled for all back-ends?)
  • tests/render/imported-parameters.hlsl: (Disabled for all back-ends?)
  • tests/render/nointerpolation.hlsl: (Disabled for all back-ends?)
  • tests/render/render0.hlsl: (Disabled for all back-ends?)
  • tests/render/unused-discard.hlsl: (Disabled for all back-ends?)

@csyonghe It seems a lot of the rendering tests are disabled for all back-ends? Why is that? Should I look into enabling them as part of this task?

For tests/compute/texture-sampling.slang and tests/compute/texture-sample-grad-offset-clamp.slang I don't think there's much we can do on WGSL.

aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 13, 2024
There was a "kinds" vs. "kind flags" mismatch, and also getBindingOffsetForKinds was not
being used.

This patch enables a bunch of tests for WGPU.
This helps to address issue shader-slang#4943.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 13, 2024
There was a "kinds" vs. "kind flags" mismatch, and also getBindingOffsetForKinds was not
being used.

This patch enables a bunch of tests for WGPU.
This helps to address issue shader-slang#4943.
csyonghe pushed a commit that referenced this issue Nov 13, 2024
* Update Slang-RHI to get WGPU backend fixes

* render-test: Use device local memory type for vertex buffers

This helps to avoid shader-slang/slang-rhi#104

* Fix bug in WGSL emitter layout code.

There was a "kinds" vs. "kind flags" mismatch, and also getBindingOffsetForKinds was not
being used.

This patch enables a bunch of tests for WGPU.
This helps to address issue #4943.

* format code

---------

Co-authored-by: slangbot <[email protected]>
@csyonghe
Copy link
Collaborator

For tests/compute/texture-sampling.slang, let's make a copy of it without the unsupported 1d texture so we can have coverage for wgpu.

I don't know why many of the tests here are disabled, but I just tried enabling tests/render/render0.hlsl locally on my machine and it worked just fine for d3d12 and d3d11. On WGPU it runs into a rhi error complaining a texture is created with wrong usage flags which I think should be easy to fix.

So let's try to enable all the currently disabled tests, and if they fail for any reason, we'll decide what to do.

aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 14, 2024
There are no 1d array textures in WGSL, so
add texture-sampling-no-1d-arrays.slang based on texture-sampling.slang, but without
1d texture arrays.

This helps to address issue shader-slang#4943.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 14, 2024
aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 14, 2024
There are no 1d array textures in WGSL, so
add texture-sampling-no-1d-arrays.slang based on texture-sampling.slang, but without
1d texture arrays.

This helps to address issue shader-slang#4943.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 15, 2024
skallweitNV added a commit to shader-slang/slang-rhi that referenced this issue Nov 15, 2024
* WGPU: Create cube textures with one layer per face

This helps to address
shader-slang/slang#4943

* Update src/wgpu/wgpu-texture.cpp

* Update src/wgpu/wgpu-texture.cpp

* Update src/wgpu/wgpu-texture.cpp

---------

Co-authored-by: Simon Kallweit <[email protected]>
@aleino-nv
Copy link
Collaborator Author

Currently looking into Slang-RHI issues with tests/compute/texture-sampling-no-1d-arrays.slang.

Slang-RHI gets the following error message when trying to create a 1d texture:

WGPU error: Texture mip level count (3) is more than 1 when its dimension is TextureDimension::e1D.
 - While validating [TextureDescriptor "Unnamed texture (type=Texture1D, size=4x1x1, arrayLength=1, mipLevelCount=3, sampleCount=1, sampleQuality=0, format=R8G8B8A8_UNORM, memoryType=DeviceLocal, usage=ShaderResource|CopySource|CopyDestination, defaultState=ShaderResource)"].
 - While calling [Device].CreateTexture([TextureDescriptor "Unnamed texture (type=Texture1D, size=4x1x1, arrayLength=1, mipLevelCount=3, sampleCount=1, sampleQuality=0, format=R8G8B8A8_UNORM, memoryType=DeviceLocal, usage=ShaderResource|CopySource|CopyDestination, defaultState=ShaderResource)"]).

Apparently, 1d textures in WebGPU can only have a single MIP level: https://www.w3.org/TR/webgpu/#abstract-opdef-maximum-miplevel-count.

The error message disappeared after I imposed the WebGPU constraint in render-test.

I can't just impose this constraint in render-test, because then other tests, that actually do want to test with multiple MIP levels, will break.

I suppose I will similarly have to customize this test so that, besides avoiding 1d arrays it also ensures that the 1d textures are only created with at most a single level. I see the Texture2D test parameters takes a mipMaps argument, so I can just set that to 1 in the simpler WebGPU test case.

@aleino-nv
Copy link
Collaborator Author

tests/compute/texture-sampling-no-1d-arrays.slang is almost passing with recent Slang-RHI fixes and when avoiding 1d textures with more than 1 MIP levels.

The remaining issue is that these two lines are apparently sampling 0s, when they should be getting 1s.

val += tCubeArray.Sample(samplerState, float4(uv, 0.5, 0.0));
val += tCube.Sample(samplerState, float3(uv, 0.5));

Currently debugging that...

aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 19, 2024
This helps to address issue shader-slang#4943.
@aleino-nv
Copy link
Collaborator Author

aleino-nv commented Nov 19, 2024

It turns out that only the +x direction face was being written repeatedly by Slang-RHI.

Here is a PR that I believe contains a fix: shader-slang/slang-rhi#110

EDIT: The PR makes the Slang texture sample test pass, but it breaks some other Slang-RHI tests. Currently debugging that.

csyonghe added a commit that referenced this issue Nov 19, 2024
* Implement semantics for WGSL

This helps to address issue #4943.

* format code

---------

Co-authored-by: slangbot <[email protected]>
Co-authored-by: Yong He <[email protected]>
aleino-nv added a commit to shader-slang/slang-rhi that referenced this issue Nov 20, 2024
westlicht pushed a commit to shader-slang/slang-rhi that referenced this issue Nov 20, 2024
aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 20, 2024
This avoids running into a WebGPU limitation, and helps to address issue shader-slang#4943.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 20, 2024
@aleino-nv
Copy link
Collaborator Author

With #5617 tests/compute/texture-sampling-no-1d-arrays.slang is now passing.

@aleino-nv
Copy link
Collaborator Author

aleino-nv commented Nov 20, 2024

For tests/compute/texture-sampling.slang, let's make a copy of it without the unsupported 1d texture so we can have coverage for wgpu.

I don't know why many of the tests here are disabled, but I just tried enabling tests/render/render0.hlsl locally on my machine and it worked just fine for d3d12 and d3d11. On WGPU it runs into a rhi error complaining a texture is created with wrong usage flags which I think should be easy to fix.

So let's try to enable all the currently disabled tests, and if they fail for any reason, we'll decide what to do.

EDIT nevermind, I found out a way to get some tests running after some more experimentation.

@csyonghe Can you elaborate a bit on how you enabled and ran tests/render/render0.hlsl?
I did the following and I just get "no tests run".

C:\Users\aleino\workspaces\slang>git diff 
diff --git a/tests/render/render0.hlsl b/tests/render/render0.hlsl
index 967f23ad..40acbae9 100644
--- a/tests/render/render0.hlsl
+++ b/tests/render/render0.hlsl
@@ -1,4 +1,3 @@
-//DISABLED_TEST(smoke):COMPARE_HLSL_RENDER:
 // Starting with a basic test for the ability to render stuff...
 
 cbuffer Uniforms

C:\Users\aleino\workspaces\slang>cd C:\Users\aleino\workspaces\slang   && C:\Users\aleino\workspaces\slang\build\Debug\bin\slang-test tests/render/render0.hlsl   || exit 1 
Supported backends: fxc dxc glslang spirv-dis visualstudio genericcpp llvm spirv-opt tint
no tests run

csyonghe added a commit that referenced this issue Nov 20, 2024
* Limit number of MIP levels on 1d textures to be 1

This avoids running into a WebGPU limitation, and helps to address issue #4943.

* Update Slang-RHI to get WGPU fixes

* Enable WGPU texture sampling test

This helps to address issue #4943.

---------

Co-authored-by: Yong He <[email protected]>
aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 21, 2024
I found that Slang-RHI/WGPU was not able to copy from render targets to staging buffers.

This helps to address issue shader-slang#4943.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Nov 21, 2024
I found that Slang-RHI/WGPU was not able to copy from render targets to staging buffers.

This helps to address issue shader-slang#4943.
@aleino-nv
Copy link
Collaborator Author

This PR allowed me to enable all tests/render for WGPU except for the nointerpolate one.

@aleino-nv
Copy link
Collaborator Author

This second PR adds some explanations for render tests we can't enable for WGSL:
#5628

With this PR and the previous one, only the nointerpolate issue is left.

csyonghe pushed a commit that referenced this issue Nov 21, 2024
* render-test: Add copy-source usage for render targets

I found that Slang-RHI/WGPU was not able to copy from render targets to staging buffers.

This helps to address issue #4943.

* Add entries to render API util infos

Entries for glsl-cross and glsl-rewrite are added.

Without glsl-cross, slang-test fails to select a back-end, and winds up crashing when
tests/render/cross-compile-entry-point.slang is enabled

tests/render/cross-compile0.hlsl fails similarly without glsl-rewrite.

* Enable some rendering tests

* Add expected test outputs
@aleino-nv
Copy link
Collaborator Author

The one remaining disabled rendering test that we should address is tracked in #5625.

Closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:forward looking Feature needed at a later date, not connected to a specific use case. kind:enhancement a desirable new feature, option, or behavior
Projects
None yet
Development

No branches or pull requests

4 participants