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

slang-test assert on usePassthru #5276

Closed
aleino-nv opened this issue Oct 14, 2024 · 5 comments · Fixed by #5315
Closed

slang-test assert on usePassthru #5276

aleino-nv opened this issue Oct 14, 2024 · 5 comments · Fixed by #5315
Assignees
Labels
goal:forward looking Feature needed at a later date, not connected to a specific use case. kind:bug something doesn't work like it should

Comments

@aleino-nv
Copy link
Collaborator

This assert triggers

    case RenderApiType::WebGPU:
        target = SLANG_WGSL;
        SLANG_ASSERT(!usePassthru);
        break;

Affected test:

  • tests/bugs/texture2d-gather.hlsl (NOTE: .hlsl extension)

I think the reason usePassthru is true is due to this special case logic above, for this particular test, due to file extension of .hlsl:

            if (languageRenderType != RenderApiType::Unknown)
            {
                foundLanguageRenderType = languageRenderType;

                // Use the pass thru compiler if these are the sources
                usePassthru |= (argName == "hlsl" || argName == "glsl");

                continue;
            }
@jkwak-work
Copy link
Collaborator

This assertion is a blocker for a PR

I am going to comment out the assert for now.

@aleino-nv
Copy link
Collaborator Author

This assertion is a blocker for a PR

I am going to comment out the assert for now.

If it's a single test triggering this due to some file-extension issue, then I think the test should just be disabled for now, rather than commenting out the assert.

@jkwak-work jkwak-work self-assigned this Oct 16, 2024
@jkwak-work jkwak-work added goal:forward looking Feature needed at a later date, not connected to a specific use case. kind:bug something doesn't work like it should labels Oct 16, 2024
@jkwak-work
Copy link
Collaborator

It appears that the assertion is incorrect and it should be removed.

The issue turned out to be related to the use of COMPARE_HLSL_RENDER keyword in the file, tests/bugs/texture2d-gather.hlsl.
The fact that the file has an extension, ".hlsl", was unrelated to the issue.

The document describes what COMPARE_HLSL_RENDER does as following,

Runs 'render-test' rendering two images - one for hlsl (expected), and one for slang saving to .png files. The images must match for the test to pass.

Basically, it compiles the given shader with DXC compiler and generate an output image.
And then it compiles with each and every render API and generate an output image and compare it to the output from DXC.

The problem is on how the input arguments are generated when rendering for metal and wgpu.
The command-line argument for rendering with DXC is, as an example,

-wgpu -hlsl -o .expected.png

And the command-line argument for rendering with WGPU API is,

-wgpu -slang -o .actual.png

The first set of arguments conflict in a way that "-wgpu" instructs to use Tint as the renderer at the same time, it tells to use DXC compile with the next argument, "-hlsl". In this case, "-wgpu" should be ignored.

The second set is kind of O.K, because "-slang" and "-wgpu" don't conflict. "-slang" instructs to treat the input as slang shader and "-wgpu" instructs to use WGPU as the output render API.

@jkwak-work
Copy link
Collaborator

jkwak-work commented Oct 17, 2024

I merged a proper fix as in PR 5315.
And the assertion failure is no longer a problem.

However, the test itself is still failing.
It will need to be addressed as another issue.

@jkwak-work
Copy link
Collaborator

To be clear, the proper fix was to remove the assertion and replace it with the following lines, which is in a same pattern as other targets in the same switch-statement.

        case RenderApiType::CUDA:
            target = SLANG_PTX;
            nativeLanguage = SLANG_SOURCE_LANGUAGE_CUDA;
            passThru = SLANG_PASS_THROUGH_NVRTC;
            break;
        case RenderApiType::WebGPU:
            target = SLANG_WGSL;
            // SLANG_ASSERT(!usePassthru); // <==== THIS IS REMOVED
            nativeLanguage = SLANG_SOURCE_LANGUAGE_WGSL; // <=== THIS IS ADDED
            passThru = SLANG_PASS_THROUGH_TINT; // <=== THIS IS ADDED
            break;
        }

    SlangSourceLanguage sourceLanguage = nativeLanguage;
    if (!usePassthru)
    {
        sourceLanguage = SLANG_SOURCE_LANGUAGE_SLANG;
        passThru = SLANG_PASS_THROUGH_NONE;
    }

As explained on my previous comment, a right behavior is to ignore passThru and nativeLanague values when -hlsl command-line argument is given.

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:bug something doesn't work like it should
Projects
None yet
2 participants