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

Add WGSL support for slang-test #5174

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aleino-nv
Copy link
Collaborator

  • Add Tint as a downstream compiler
  • Add some plumbing for slang-test to target WebGPU and WGSL using the newly added RHI support
  • Disable several asserting slang-test tests in tests/compute

@aleino-nv aleino-nv added the pr: non-breaking PRs without breaking changes label Sep 27, 2024
@aleino-nv
Copy link
Collaborator Author

@csyonghe Should I use the breaking label instead? I'm adding some stuff to the middle of an enum in slang.h.
Maybe I should just move that value to the end?

@aleino-nv
Copy link
Collaborator Author

There are some things that seem a bit strange about this: I have added "WGSL SPIR-V" and "WGSL SPIR-V assembly" targets in a bunch of places just to be able to express that I want Tint to produce the SPIR-V.

@csyonghe
Maybe there are other mechanisms to achieve this that don't involve adding these strange "redundant" targets?

external/slang-tint-headers/slang-tint.h Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.h Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.h Outdated Show resolved Hide resolved
source/core/slang-render-api-util.cpp Show resolved Hide resolved
source/slang/slang.cpp Show resolved Hide resolved
tests/compute/atomics-buffer.slang Show resolved Hide resolved
@csyonghe
Copy link
Collaborator

To answer your questions: all enums need to be added at the end to avoid binary breakage. Defining wasm-spirv and wasm-spirv-asm as separate targets is fine.

@csyonghe
Copy link
Collaborator

Instead of adding DISABLED_TEST lines to individual .slang files, please add the test names to tests/expected-failure-github.txt so we can use the list as a centralized place to track which tests are being disabled.

@aleino-nv
Copy link
Collaborator Author

Instead of adding DISABLED_TEST lines to individual .slang files, please add the test names to tests/expected-failure-github.txt so we can use the list as a centralized place to track which tests are being disabled.

I can do that, but these tests are asserting which seems to interrupt the test run a lot. Disabling gets rid of that problem.

Do you still think I should just mark these as test failures?

@aleino-nv
Copy link
Collaborator Author

Instead of adding DISABLED_TEST lines to individual .slang files, please add the test names to tests/expected-failure-github.txt so we can use the list as a centralized place to track which tests are being disabled.

I can do that, but these tests are asserting which seems to interrupt the test run a lot. Disabling gets rid of that problem.

Do you still think I should just mark these as test failures?

As discussed on Slack I will keep these disabled and then also add to the expected-failure list.

sourceArtifact->loadBlob(ArtifactKeep::Yes, sourceBlob.writeRef())
);

std::string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use String instead of std::string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved

tint_CompileResult result = {};
int const resultCode {m_compile(&req, &result)};
if (resultCode != 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leaking memory on tint_compileResult.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Use SLANG_DEFER to ensure things are freed:

auto rs = m_compile(&result);
SLANG_DEFER(m_free(&result));
if (rs != 0) ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean m_compile fails? A common convention (which is followed in this case) is to make sure that no resources are actually returned if the call fails.

extern "C"
SLANG_SHARED_LIB_EXPORT
int tint_compile(tint_CompileRequest* request, tint_CompileResult* result)
{
    ///...
    uint8_t *const resultBuffer {static_cast<uint8_t*>(::malloc(resultBufferSize))};
    if(resultBuffer == nullptr)
        return 1;
    ::memcpy(resultBuffer, spirvCode.data(), resultBufferSize);

    result->buffer = resultBuffer;
    result->bufferSize = resultBufferSize;
    return 0;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(But of course I may as well add a DEFER anyway.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we report back diagnostics returned by tint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/core/slang-render-api-util.cpp Show resolved Hide resolved
source/slang/slang.cpp Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/core/slang-render-api-util.cpp Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.h Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.h Outdated Show resolved Hide resolved
source/slang/slang.cpp Show resolved Hide resolved
tests/compute/atomics-buffer.slang Show resolved Hide resolved
@csyonghe
Copy link
Collaborator

csyonghe commented Oct 2, 2024

On coding style:
No ); on its own line.
No ( on its own line.
No const unless necessary (required by c++ standard).
No T x{} initialization. Should be T c= {}
No return on its own line.

Long function signature should either be

Blah longFunc(
    All parameters);

Or

Blah longFunc(
   Param1
   ParamN);

source/compiler-core/slang-glslang-compiler.cpp Outdated Show resolved Hide resolved
external/slang-tint-headers/slang-tint.h Outdated Show resolved Hide resolved
source/compiler-core/slang-glslang-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
source/core/slang-render-api-util.cpp Outdated Show resolved Hide resolved
@aleino-nv aleino-nv force-pushed the aleino/wgsl branch 2 times, most recently from 415aff1 to 77ed29d Compare October 3, 2024 10:19
csyonghe
csyonghe previously approved these changes Oct 3, 2024
source/compiler-core/slang-tint-compiler.cpp Outdated Show resolved Hide resolved
I believe this is a bugfix.
It seems to have worked before because up until the WGSL case, the disassembler has been
the same executable as the one producing the binary to be disassembled.
This closes issue shader-slang#5104.

* Add downstream compiler for Tint.
 * Tint is wrapped in a shared library, 'slang-tint' available from [1].
 * The header file for slang-tint.dll is added in external/slang-tint-headers.
* Add some boilerplate for WGSL targets.
* Add an entry point test for WGSL.

[1] https://github.com/shader-slang/dawn/releases/tag/slang-tint-0
@csyonghe
Copy link
Collaborator

csyonghe commented Oct 4, 2024

Reading the ci log it doesn't seem wgpu is reported as supported and no tests are run for wgpu.

I merged Simon's and see if ci can pick up the tests this time.

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.

2 participants