Skip to content
Snippets Groups Projects

tests: Improvements for SM1 testing.

Merged Francisco Casas requested to merge fcasas/vkd3d:snk_games2_part into master
4 unresolved threads

This is the first part of !608 (merged), which I decided to separate to ease review.

It consists of improvements to SM1 testing, mainly:

  • Correctly passing int and bool uniforms as IEEE 754 floats to SM1 profiles.
  • The introduction of the "only" qualifier to avoid abusing [require] blocks in case tests retrieve different results for different shader models.
  • Allowing some of the tests we already have for SM4/SM6 to run in SM1 after those improvements.

Merge request reports

Merge request pipeline #22387 skipped

Merge request pipeline skipped for 6a8939e1

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Feb 13, 2024 10:11pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Francisco Casas mentioned in merge request !608 (merged)

    mentioned in merge request !608 (merged)

    • Part of the point of passing bools as a value other than 1 or 0 there is that it validated that sm4 was doing the right thing in flushing them to 1 and 0. Why not just flush for sm1 in the shader runner like in 3/5?

      Also, regardless, I don't think we need a separate "bool" type, we can just use int or uint.

    • Yeah, I mostly see the interface between the CPU program and the shaders as binary, not typed. Specifying a type in the .shader_test file is a convenience for humans, but I don't think there should be any magic happening. Rather, I'd use the only() syntax you just introduced to specify different inputs depending on the SM:

      only(sm<4) uniform 0 float4 1.0 2.0 3.0 4.0
      only(sm>=4) uniform 0 uint4 1 2 3 4

      How does that feel?

    • Author Developer

      This seems reasonable to me, we just have to remember to pass bools and ints as float for sm<4, and in particular, pass true as float 1.0.

    • Please register or sign in to reply
    • +% The following test doesn't work on SM1 because, on it, each uniform has the whole register.

      We should just change the uniform layout then.

    • Author Developer

      Since we will be passing ints using the "only" qualifier as Giovanni suggested, we can now just provide different offsets for the different shader models.

      So something like this:

      uniform 0 int -1
      uniform 1 uint 3
      uniform 2 int -2
      uniform 3 float 0.5

      ends ups like this:

      if(sm<4) uniform 0 float -1
      if(sm<4) uniform 4 float 3
      if(sm<4) uniform 8 float 1
      if(sm<4) uniform 12 float 0.5
      if(sm>=4) uniform 0 int -1
      if(sm>=4) uniform 1 uint 3
      if(sm>=4) uniform 2 int -2
      if(sm>=4) uniform 3 float 0.5
    • Please register or sign in to reply
    • From 78ecef0ccf024fa728c952c13cf0c885c61b3a6f Mon Sep 17 00:00:00 2001
      From: Francisco Casas <fcasas@codeweavers.com>
      Date: Mon, 29 Jan 2024 20:07:39 -0300
      Subject: [PATCH 2/5] tests/shader-runner: Introduce "only" qualifier.
      
      When the "only" qualifier is added to a directive, the directive is
      skipped if the shader->minimum_shader_model is not included in the
      range.
      
      This can be used on the "probe" directives for tests that have different
      expected results on different shader models, without having to resort to
      [require] blocks.

      I think most languages call that "if"? I.e.:

      if(sm<6) probe ...
      if(sm>=6) probe ...

      Not that I mind doing something different, but I suppose it might make things slightly harder to pick up for someone new to writing .shader_test's.

      Some possible alternatives:

      probe(sm<6) ...
      probe(sm>=6) ...
      cond
          (sm<6) probe ...
          (sm>=6) probe ...
    • Author Developer

      Okay, I renamed the "only" qualifiers as "if". Hopefully we won't use these too enough to justify having to add conditional blocks.

    • Please register or sign in to reply
  • Francisco Casas added 4 commits

    added 4 commits

    • 2088cf12 - tests/vkd3d-shader: Set uninitialized uniforms to zero.
    • 5989e6b9 - tests: Don't ignore SM1 on a non-const-indexing.shader_test test.
    • 2676b0bd - tests/shader-runner: Introduce "if" qualifier.
    • 9d62b88b - tests: Remove [require] directives for tests that use int and bool uniforms.

    Compare with previous version

  • Author Developer

    I updated according to the suggestions.

    There are two conflicting ideas of how to pass uniform values to the backend:

    a) Allow the shader_runner to perform transformations of the input values according to the backend and the type provided (so it makes sense to add the "bool" type to the "uniform" directives)

    b) Don't allow the shader_runner to perform these transformations, and instead pass the values as if they where binary data, needing the use of more "only" (now called "if") qualifiers.

    The first version of this MR was doing (a) but the current one does (b). The disadvantage of (b) is that we have to add more "if" qualifiers, if you think these are too many I can revert to (a).

    Edited by Francisco Casas
    • @@ -540,9 +540,12 @@ static void set_resource(struct shader_runner *runner, struct resource *resource
       
       static void set_uniforms(struct shader_runner *runner, size_t offset, size_t count, const void *uniforms)
       {
      +    size_t initial_capacity = runner->uniform_capacity;
      +
           runner->uniform_count = align(max(runner->uniform_count, offset + count), 4);
           vkd3d_array_reserve((void **)&runner->uniforms, &runner->uniform_capacity,
                   runner->uniform_count, sizeof(*runner->uniforms));
      +    memset(runner->uniforms + initial_capacity, 0, runner->uniform_capacity - initial_capacity);
           memcpy(runner->uniforms + offset, uniforms, count * sizeof(*runner->uniforms));
       }

      How does that work? Are we sending more than "uniform_count" uniforms to the shader anywhere? Or is this about the potential padding introduced by align? In the latter case, we need to clear a fair bit less than we do here. We may also want to consider clearing with a value other than zero.

      The first version of this MR was doing (a) but the current one does (b). The disadvantage of (b) is that we have to add more "if" qualifiers, if you think these are too many I can revert to (a).

      We could conceivably introduce additional types that do type conversion when targetting d3dbc if the current scheme becomes too cumbersome, but for the moment this seems fine.

    • Author Developer

      How does that work? Are we sending more than "uniform_count" uniforms to the shader anywhere? Or is this about the potential padding introduced by align?

      Both in SM1 or when we work with arrays we commonly declare uniforms leaving the padding uninitialized:

      uniform 0 float 1.0
      uniform 4 float 2.0
      uniform 8 float 3.0

      so the aim is to initialize it, including the padding after the last uniform declared.

      My idea was to initialize the new values in the array after a reallocation happens, before inserting the values provided at the offset position, to ensure that we initialize the padding.

      In the latter case, we need to clear a fair bit less than we do here.

      Ah, I see it now.

      We may also want to consider clearing with a value other than zero.

      Yep, I think it makes sense to initialize to a sentry value other than zero, maybe 127 ?

      btw, I forgot to multiply the amount of bytes by the size of each uniform. So the final instruction should be:

      memset(runner->uniforms + initial_count, 127,
              (runner->uniform_count - initial_count) * sizeof(*runner->uniforms));
      Edited by Francisco Casas
    • Please register or sign in to reply
  • Francisco Casas added 4 commits

    added 4 commits

    • 743c6f6d - tests/vkd3d-shader: Set uninitialized uniforms to zero.
    • 5fd06dea - tests: Don't ignore SM1 on a non-const-indexing.shader_test test.
    • eda136d4 - tests/shader-runner: Introduce "if" qualifier.
    • 70b13227 - tests: Remove [require] directives for tests that use int and bool uniforms.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • This needs a rebase, but looks otherwise ok to me.

  • Subject: [PATCH 1/4] tests/vkd3d-shader: Set uninitialized uniforms to zero.

    That's not quite accurate now that we're clearing with 0x7f.

  • Francisco Casas added 79 commits

    added 79 commits

    • 70b13227...5eba031f - 75 commits from branch wine:master
    • dbf614c5 - tests/vkd3d-shader: Set uninitialized uniforms to zero.
    • 65f44896 - tests: Don't ignore SM1 on a non-const-indexing.shader_test test.
    • a01c80c3 - tests/shader-runner: Introduce "if" qualifier.
    • d8398170 - tests: Remove [require] directives for tests that use int and bool uniforms.

    Compare with previous version

  • Francisco Casas added 4 commits

    added 4 commits

    • 5a3a406e - tests/vkd3d-shader: Set uninitialized uniforms to a value.
    • 1f42d8cf - tests: Don't ignore SM1 on a non-const-indexing.shader_test test.
    • 891efa26 - tests/shader-runner: Introduce "if" qualifier.
    • ad6df0ed - tests: Remove [require] directives for tests that use int and bool uniforms.

    Compare with previous version

  • @@ -545,15 +572,17 @@ float4 main() : sv_target
     }
     
     [test]
    -uniform 0 uint4 1 0 3 1
    +if(sm<4)  uniform 0 float4 0 0 3 1
    +if(sm>=4) uniform 0 uint4 0 0 3 1
     draw quad
    -probe all rgba (10.0, 11.0, 12.0, 13.0)
    -uniform 0 uint4 0 0 3 1
    +todo(sm>=4) probe all rgba (10.0, 11.0, 12.0, 13.0)
    +if(sm<4)  uniform 0 float4 1 0 3 1
    +if(sm>=4) uniform 0 uint4 1 0 3 1

    That looks like a rebase glitch after 4ba324d3.

  • Francisco Casas added 1 commit

    added 1 commit

    • d172e78d - tests: Remove [require] directives for tests that use int and bool uniforms.

    Compare with previous version

  • Francisco Casas added 1 commit

    added 1 commit

    • 9cb6eb9c - tests: Remove [require] directives for tests that use int and bool uniforms.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 4 commits

    • 0f9f5269 - tests/vkd3d-shader: Set uninitialized uniforms to a value.
    • faec42e8 - tests: Don't ignore SM1 on a non-const-indexing.shader_test test.
    • 22c47e57 - tests/shader-runner: Introduce "if" qualifier.
    • 6a8939e1 - tests: Remove [require] directives for tests that use int and bool uniforms.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading