Skip to content
Snippets Groups Projects

tests/shader-runner: Test DXIL shaders.

Merged Conor McCarthy requested to merge cmccarthy/vkd3d:dxil_tests into master

This goes on top of MR 345.

Edited by Conor McCarthy

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • I haven't reviewed in depth yet, but for the moment I wouldn't mix shader runner tests between SM1-5 and SM6. The SM6 parser is still so crude that it fails on basically everything; also, running SM6 tests requires installing a separate binary component, which developers not interested about SM6 might not have. This would make the handling of todo_6 and similar annotations more complicated, and for very little useful signal.

    For the moment I'd rather keep SM6 tests separated from all the others, and merge them once the SM6 parser allow a decent amount of them to pass.

    Also, I would like to have some appropriate abstractions in the shader runner (and in the other vkd3d tests) so that SM6 support is not a special case everywhere, but rather the shader runner itself has support for having different shader compilers (vkd3d-shader, d3dcompiler_xx.dll, dxcompiler.dll) and for targeting different profiles in somehow the same way it now allows using different execution environments (that that support too is still to be perfected). Using SM6 and dxcompiler.dll should just be another case along these axes, rather than an exceptional code flow everywhere.

    I had already written some code to this end, I can try to dust it off and submit a proposal. Of course perspectives on how that could work are welcome.

    What do you think of all of this?

    • I haven't reviewed in depth yet, but for the moment I wouldn't mix shader runner tests between SM1-5 and SM6. The SM6 parser is still so crude that it fails on basically everything; also, running SM6 tests requires installing a separate binary component, which developers not interested about SM6 might not have. This would make the handling of todo_6 and similar annotations more complicated, and for very little useful signal.

      For the moment I'd rather keep SM6 tests separated from all the others, and merge them once the SM6 parser allow a decent amount of them to pass.

      I don't think that's a problem per se; I think it's fine to annotate some tests with todo/skip/fail based on e.g. the shader model or possible a specific runner. The CI can hopefully help avoid the need for each individual contributor to have a working dxcompiler installation, though I could certainly see it being a bit of a pain when adding new tests, certainly in the beginning when essentially nothing works.

      I would like something a bit more coherent than "fail_tpf", "skip_dxil", and "todo_6" though. How about something along the lines of "fail(shader model < 6.0)" and "todo(shader model >= 6.0)"? "skip_dxil" sounds like it should simply be a "shader model < 6.0" line in the "[require]" section.

      Also, I would like to have some appropriate abstractions in the shader runner (and in the other vkd3d tests) so that SM6 support is not a special case everywhere, but rather the shader runner itself has support for having different shader compilers (vkd3d-shader, d3dcompiler_xx.dll, dxcompiler.dll) and for targeting different profiles in somehow the same way it now allows using different execution environments (that that support too is still to be perfected). Using SM6 and dxcompiler.dll should just be another case along these axes, rather than an exceptional code flow everywhere.

      Essentially, yes. I'm not necessarily convinced that we fully need to resolve that as part of this MR, but plumbing through a "use_dxcompiler" flag everywhere doesn't seem like a step in the right direction; I'd already feel a fair bit better about this if it was expressed in terms of minimum/maximum shader models. I.e., something along the lines of

          run_shader_tests_d3d12(SHADER_MODEL_4_0, SHADER_MODEL_5_1);
          run_shader_tests_d3d12(SHADER_MODEL_6_0, SHADER_MODEL_6_0);

      instead of

          run_shader_tests_d3d12(false);
          run_shader_tests_d3d12(true);

      There are a bunch of style issues in dxcompiler.idl, which I think should be fairly self-evident at this point. Beyond those though, why do we need that to be a public header?

      The shader runner changes at first sight look like multiple changes in a single commit.

      You'll probably want to use vkd3d_dlopen() and vkd3d_dlsym(), much like the Vulkan runner does.

      It's perhaps worth pointing out that "8ba5fb08-5195-40e2-ac58-0d989c3a0102" is also the uuid for ID3D10Blob/ID3DBlob. It looks like IDxcBlob is a simple alias.

    • Author Developer

      It is, hence dxcompiler_compile_shader() returns an IDxcBlob as an ID3D10Blob. However they are not quite compatible; see the comment in compile_shader() on using the return value of GetBufferPointer() as a string.

    • Author Developer

      Beyond those though, why do we need that to be a public header?

      I couldn't get it to use vkd3d_windows.h from within tests. I need to fix that at some point.

      "skip_dxil" sounds like it should simply be a "shader model < 6.0" line in the "[require]" section.

      Ironically I started with this and thought it may to too restrictive; I'll need to reorder the tests which need this so they come last in their source file, but it's rare enough not to be an issue.

    • Essentially, yes. I'm not necessarily convinced that we fully need to resolve that as part of this MR, but plumbing through a "use_dxcompiler" flag everywhere doesn't seem like a step in the right direction; I'd already feel a fair bit better about this if it was expressed in terms of minimum/maximum shader models. I.e., something along the lines of

          run_shader_tests_d3d12(SHADER_MODEL_4_0, SHADER_MODEL_5_1);
          run_shader_tests_d3d12(SHADER_MODEL_6_0, SHADER_MODEL_6_0);

      instead of

          run_shader_tests_d3d12(false);
          run_shader_tests_d3d12(true);

      Yeah, that's already better. It still conflates the SM and the compiler, which is a thing I'd like to eventually disentangle, but that doesn't need to happen in this MR, and I can take responsibility for that.

    • Please register or sign in to reply
    • I would like something a bit more coherent than "fail_tpf", "skip_dxil", and "todo_6" though. How about something along the lines of "fail(shader model < 6.0)" and "todo(shader model >= 6.0)"? "skip_dxil" sounds like it should simply be a "shader model < 6.0" line in the "[require]" section.

      I agree something more coherent would be nice, but that seems excessively verbose to me. I don't think we're going to need any more complex categories than "sm1", "sm4", and "sm6".

      Wrt fail_tpf... what may make sense is, every time we have a require directive for a shader version, we try to compile it with every version anyway, and validate that HLSL compilation fails for versions outside of the range. Then that would become

      [require] shader model >= 6.0

      Although you do mention that some tests ICE the native compiler, so I'm not sure how to deal with that...

      Somewhat relatedly, for some of those tests I don't even know how the relevant shader would be interpreted, so we may want an actual [test] directive following them.

      "skip_dxil" sounds like it should simply be a "shader model < 6.0" line in the "[require]" section.

      Ironically I started with this and thought it may to too restrictive; I'll need to reorder the tests which need this so they come last in their source file, but it's rare enough not to be an issue.

      No, we should probably just change [require] so that it doesn't skip the whole file.

      That or move the relevant tests to separate files. Embedding lots of tests in the same file is... the problem with it is that, somewhat unlike C, it's not the easiest thing to read; I blame the lack of syntax highlighting support.

    • Author Developer

      I agree something more coherent would be nice, but that seems excessively verbose to me. I don't think we're going to need any more complex categories than "sm1", "sm4", and "sm6".

      I favour less verbosity too. It can be, e.g. todo(sm4) or a list like todo(sm4,sm6), with plain todo applying to all.

    • Please register or sign in to reply
  • Another thing that would be appropriate to add to this MR would be a paragraph of instructions on how to fetch and/or compile dxcompiler.dll and to make it available to the test program. And also doing that thing for the CI pipeline (I can assist with that if necessary).

  • Conor McCarthy added 4 commits

    added 4 commits

    • a4bd490e - tests/shader-runner: Do not exit if a 'require' directive is not met.
    • 43d9317c - tests/shader-runner: Replace immediate shader type strings with an enum.
    • aa079aac - tests/shader-runner: Test shaders with dxcompiler.
    • 1805aa58 - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Conor McCarthy added 12 commits

    added 12 commits

    • 1805aa58...80ff1f87 - 2 earlier commits
    • db54961c - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
    • 0613680a - vkd3d-shader/dxil: Declare shader inputs.
    • 2e071572 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
    • 0091f197 - vkd3d-shader/dxil: Do not access null code blocks on failure.
    • f79b0096 - vkd3d-shader/dxil: Do not compile compute shaders.
    • 2b685f78 - tests/shader-runner: Handle individual keywords in shader directives.
    • e2360dcc - tests/shader-runner: Do not exit if a 'require' directive is not met.
    • 91ca0c28 - tests/shader-runner: Replace immediate shader type strings with an enum.
    • bf5eb7d7 - tests/shader-runner: Test shaders with dxcompiler.
    • cc5ac663 - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Conor McCarthy added 5 commits

    added 5 commits

    • 90652afc - tests/shader-runner: Handle individual keywords in shader directives.
    • 57126795 - tests/shader-runner: Do not exit if a 'require' directive is not met.
    • f5d668c2 - tests/shader-runner: Replace immediate shader type strings with an enum.
    • 5fcd94b3 - tests/shader-runner: Test shaders with dxcompiler.
    • 193394ef - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Conor McCarthy added 2 commits

    added 2 commits

    • 807f5e93 - tests/shader-runner: Test shaders with dxcompiler.
    • 738d134b - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Author Developer

    It turns out HasOutput() often returns true after compilation failure. Returning E_FAIL if the blob buffer size is zero results in dozens of test failures which I need to correct.

  • Conor McCarthy added 27 commits

    added 27 commits

    • 738d134b...46c7f65b - 15 commits from branch wine:master
    • 46c7f65b...baaf7b6e - 2 earlier commits
    • 5d9fa52b - vkd3d-shader/spirv: Introduce a Static Single Assignment register type.
    • 4df09e6e - vkd3d-shader/spirv: Build undefined values once.
    • 7b91e979 - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
    • 266ba19f - vkd3d-shader/dxil: Declare shader inputs.
    • 6badb521 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
    • 6a58ef7b - tests/shader-runner: Handle individual keywords in shader directives.
    • b7376f6e - tests/shader-runner: Do not exit if a 'require' directive is not met.
    • 1ab4ebd5 - tests/shader-runner: Replace immediate shader type strings with an enum.
    • 64ab31f2 - tests/shader-runner: Test shaders with dxcompiler.
    • d78ac338 - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Conor McCarthy added 12 commits

    added 12 commits

    • d78ac338...7975227d - 2 earlier commits
    • 873c9a4e - vkd3d-shader/spirv: Introduce a Static Single Assignment register type.
    • ab37b6e2 - vkd3d-shader/spirv: Build undefined values once.
    • 9e073ce5 - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
    • 76e59dc5 - vkd3d-shader/dxil: Declare shader inputs.
    • f2e6f2e4 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
    • aff611e1 - tests/shader-runner: Handle individual keywords in shader directives.
    • 89a8164d - tests/shader-runner: Do not exit if a 'require' directive is not met.
    • 64e3baeb - tests/shader-runner: Replace immediate shader type strings with an enum.
    • fbb789cd - tests/shader-runner: Test shaders with dxcompiler.
    • 9b36e00a - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Author Developer

    For some bizarre reason not immediately obvious, using the ID3D10Blob_Release() macro on an IDxcBlob caused vkd3d_test_state.skip_count to be decremented. WIDL_C_INLINE_WRAPPERS fixes this.

  • Conor McCarthy added 2 commits

    added 2 commits

    • aef973c8 - tests/shader-runner: Test shaders with dxcompiler.
    • 3b7239d5 - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Conor McCarthy added 46 commits

    added 46 commits

    • 3b7239d5...ca05e57e - 34 commits from branch wine:master
    • ca05e57e...262a1671 - 2 earlier commits
    • 0943bd0f - vkd3d-shader/spirv: Introduce a Static Single Assignment register type.
    • 2fbe46d8 - vkd3d-shader/spirv: Build undefined values once.
    • 6c5a9747 - vkd3d-shader/dxbc: Load input signatures also from ISG1 chunks.
    • 8832d895 - vkd3d-shader/dxil: Declare shader inputs.
    • 16ebb250 - vkd3d-shader/dxil: Implement DX instruction LoadInput.
    • 26d00971 - tests/shader-runner: Handle individual keywords in shader directives.
    • 1de4c03b - tests/shader-runner: Do not exit if a 'require' directive is not met.
    • 01f87a17 - tests/shader-runner: Replace immediate shader type strings with an enum.
    • 28310221 - tests/shader-runner: Test shaders with dxcompiler.
    • 0afea9cd - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Conor McCarthy changed the description

    changed the description

  • Conor McCarthy added 2 commits

    added 2 commits

    • 8ddc4dca - tests/shader-runner: Test shaders with dxcompiler.
    • 5635ad98 - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Henri Verbeet mentioned in merge request !345 (merged)

    mentioned in merge request !345 (merged)

    • Note that this MR is failing CI.

      +enum shader_type
      +{
      +    SHADER_TYPE_CS,
      +    SHADER_TYPE_GS,
      +    SHADER_TYPE_HS,
      +    SHADER_TYPE_PS,
      +    SHADER_TYPE_VS,
      +};

      This effectively adds hull and geometry shaders. I suppose that's fine, although it does mean the commit does a bit more than the commit message suggests. But if we're going to add those, why not add domain shaders as well then?

      diff --git a/tests/dxcompiler.idl b/tests/dxcompiler.idl
      new file mode 100644
      index 000000000..83b0ed5cd
      --- /dev/null
      +++ b/tests/dxcompiler.idl
      @@ -0,0 +1,155 @@
      +import "vkd3d_windows.h";
      +#include "vkd3d_unknown.idl"

      That's missing our usual copyright header.

      +cpp_quote("#ifdef _WIN32")
      +cpp_quote("#ifndef DXC_API_IMPORT")
      +cpp_quote("#define DXC_API_IMPORT __declspec(dllimport)")
      +cpp_quote("#endif")
      +cpp_quote("#else")
      +cpp_quote("#ifndef DXC_API_IMPORT")
      +cpp_quote("#define DXC_API_IMPORT __attribute__ ((visibility (\"default\")))")
      +cpp_quote("#endif")
      +cpp_quote("#endif")

      I don't think these are used?

      +/* Strip __attribute__((ms_abi)) defined in vkd3d_windows.h as dxcompiler does not use it. */
      +cpp_quote("#ifdef __x86_64__")
      +cpp_quote("# undef __stdcall")
      +cpp_quote("# define __stdcall")
      +cpp_quote("#endif")

      That's going to affect subsequent code as well.

      +interface IDxcBlobEncoding : IDxcBlob
      +{
      +    HRESULT GetEncoding(BOOL *pKnown, UINT32 *pCodePage);
      +}

      That's not our usual convention for parameter names. Affects IDxcIncludeHandler, IDxcOperationResult, IDxcResult, and IDxcCompiler3 as well.

      +typedef struct DxcDefine
      +{
      +    const WCHAR *Name;
      +    const WCHAR *Value;
      +} DxcDefine;
      +

      This seems to be unused.

      @@ -478,13 +493,22 @@ static void read_uint4(const char **line, struct uvec4 *v)
       
       static void parse_test_directive(struct shader_runner *runner, const char *line)
       {
      +    bool use_dxcompiler = runner->minimum_shader_model >= SHADER_MODEL_6_0;
           char *rest;
           int ret;
       
           runner->is_todo = false;
      +    runner->is_todo_6 = false;
       
           if (match_string(line, "todo", &line))
      +    {
      +        runner->is_todo = !use_dxcompiler;
      +        runner->is_todo_6 = use_dxcompiler;
      +    }
      +    else if (match_string(line, "todo(sm<6)", &line) && !use_dxcompiler)
               runner->is_todo = true;
      +    else if (match_string(line, "todo(sm>=6)", &line) && use_dxcompiler)
      +        runner->is_todo_6 = true;
       
           if (match_string(line, "dispatch", &line))
           {

      That just looks confusing to me. What I'd expect here is essentially something like this:

          if (match_string(line, "todo", &line))
              runner->is_todo = true;
          else if (match_string(line, "todo(sm<6)", &line))
              runner->is_todo = runner->minimum_shader_model < SHADER_MODEL_6_0;
          else if (match_string(line, "todo(sm>=6)", &line))
              runner->is_todo = runner->minimum_shader_model >= SHADER_MODEL_6_0;

      Something similar comes up in a number of other places as well. Fundamentally, I don't think the shader runner should be concerned about where a particular backend runner is using dxcompiler or not.

      +#ifdef _WIN32
      +static const char dxcompiler_name[] = "dxcompiler.dll";
      +#else
      +static const char dxcompiler_name[] = "libdxcompiler.so";
      +#endif

      We could probably introduce SONAME_LIBDXCOMPILER for this.

      From 5635ad98aa3303a1a7d00080d63afe8f4d683747 Mon Sep 17 00:00:00 2001
      From: Conor McCarthy <cmccarthy@codeweavers.com>
      Date: Thu, 14 Sep 2023 19:42:57 +1000
      Subject: [PATCH 12/12] tests/shader-runner: Add a '--dump-dxil' command line
       switch.
      
      Dumps DXIL disassembly when shaders are compiled with dxcompiler.

      I suppose that's fine. I wonder though, does this need to be specific to DXIL? If this is useful, I don't think there's a reason we couldn't support dumping disassembly for shaders compiled by d3dcompiler or vkd3d-shader as well?

    • Author Developer

      I suppose that's fine. I wonder though, does this need to be specific to DXIL? If this is useful, I don't think there's a reason we couldn't support dumping disassembly for shaders compiled by d3dcompiler or vkd3d-shader as well?

      It's especially useful for DXIL because it's under development. Maybe that could apply to d3dbc too, but I don't think plumbing a shader dump for other binaries would be simple enough to be part of this MR.

    • Please register or sign in to reply
  • Conor McCarthy added 28 commits

    added 28 commits

    • 5635ad98...d9c8b49e - 19 commits from branch wine:master
    • fe0c6a25 - vkd3d-shader/dxil: Do not access null code blocks on failure.
    • 392892f3 - vkd3d-shader/dxil: Do not compile compute shaders.
    • 50107897 - vkd3d-shader/dxil: Convert into an error the warning for an unhandled instrinsic.
    • b78381d8 - tests/shader-runner: Handle individual keywords in shader directives.
    • 4888e2c7 - tests/shader-runner: Do not exit if a 'require' directive is not met.
    • 70f83a11 - tests/shader-runner: Replace immediate shader type strings with an enum.
    • fe2eb749 - tests/shader-runner: Do not apply todo to result probes.
    • 0cf4b5d6 - tests/shader-runner: Test shaders with dxcompiler.
    • 4f25a5ce - tests/shader-runner: Add a '--dump-dxil' command line switch.

    Compare with previous version

  • Author Developer

    I renamed is_todo_6 to is_todo_pipeline to reflect that it has a different meaning than is_todo.

    I added a new patch to remove the application of is_todo to result probes. The reasoning in explained in the commit.

    dxcompiler sometimes emits a binary on failure. In some cases it reflects the erroneous HLSL, e.g. when numthreads[(0, 1, 1)] is used, but in others the binary does something else, e.g. when uniforms are used in the main() parameter list it loads them as signature inputs. The IDxcResult parent IDxcOperationResult interface contains a GetStatus() function which returns correct failure information.

    Many shaders use uniform parameters, so these now have fail(sm>=6) tags. A later patch should probably move all of these uniforms out of the parameter list, and add a single test file to test uniform parameters in SM < 6. We don't need this yet as uniforms require loading of resource info and implementation of cbuffer loads.

    This MR now depends on MR 345, mainly to make it unnecessary to have todos on result probes where DX intrinsics are unimplemented (i.e. where emit_unhandled() is called).

    A test in entry-point-semantics.shader_test fails because it depends on the DXIL shader signature. We could skip it or wait for the next MR which will load signatures.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading