Skip to content
Snippets Groups Projects

vkd3d-shader: Add compiler option to specify matrix majority.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:matrix_majority_option into master

Signed-off-by: Nikolay Sivov nsivov@codeweavers.com

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
    • This is only supported/used for HLSL sources, and I think the documentation should reflect that. (See e.g. the documentation for VKD3D_SHADER_COMPILE_OPTION_WRITE_TESS_GEOM_POINT_SIZE.) The documentation for some of the existing compilation options probably has room for improvement as well.

    • Author Developer

      Thanks, I pushed a comment for that.

    • Please register or sign in to reply
  • Nikolay Sivov added 1 commit

    added 1 commit

    • 0cba1f8a - vkd3d-shader: Add compiler option to specify matrix majority.

    Compare with previous version

  • At first sight this doesn't look wrong, but previous tests (see tests/hlsl/majority_*.shader_test) show that the way majority interacts with pragma's and typedef's is quite complicated. In particular, there are some subtle differences between having column majority as a result of setting it explicitly via a pragma and as a result of not having set any other default. How that interacts with the majority set via a compilation flag is therefore not necessarily obvious (i.e., is explicitly setting a column majority via a compilation flag equivalent to setting it via pragma, or to leaving it to the unset default?), and for that reason it would be particularly appropriate to have tests for that.

  • Author Developer

    The only reasonably fast way to test this would be with d3dcompiler directly.

  • Yes, we need tests for this. In hlsl_d3d12.c would work, but we also can't be afraid to add tests to the shader-runner infrastructure just because there's no support there yet.

  • Author Developer

    Options do work differently, they simply set default packing and produce error if different packing was specified in some other way.

  • Author Developer

    I posted some tests as wine!3263 (merged), what I think that shows is compiling options acting exactly as corresponding pragmas, when placed in front of the source file. Following pragmas will override the options, conflicting modifiers will produce errors, same as pragmas should be doing already.

    Another aspect is that you can't have both options on, but I think it's easier to filter that on wine side, keeping unnecessary validation out of the library.

  • Nikolay Sivov added 49 commits

    added 49 commits

    • 0cba1f8a...aec9ea08 - 46 commits from branch wine:master
    • 30fabe88 - vkd3d-shader: Add compiler option to specify matrix majority.
    • cf112f29 - tests: Add test file syntax to specify compiler options.
    • 193bf43a - tests: Add some tests for matrix packing compiler options.

    Compare with previous version

  • Author Developer

    Now with some tests, please take a look.

  • The test syntax looks fine, thanks. It'll still need to be hooked up to the Vulkan runner, and the tests currently pass independently of the flags.

  • Author Developer

    On Windows it does affect results. I'll see what's going on Linux/vulkan.

  • Nikolay Sivov added 4 commits

    added 4 commits

    • d4b5e79c - 1 commit from branch wine:master
    • c9fca4a7 - vkd3d-shader: Add compiler option to specify matrix majority.
    • 6b3d36b8 - tests: Add test file syntax to specify compiler options.
    • 66540e3d - tests: Add some tests for matrix packing compiler options.

    Compare with previous version

  • Author Developer

    I see what you mean now, I adjusted tests to show the difference. And to work correctly with pre-sm4 target that insist on allocating adjusting uniform register sizes based on its usage (https://bugs.winehq.org/show_bug.cgi?id=55207).

  • Nikolay Sivov added 21 commits

    added 21 commits

    • 66540e3d...e0e261ea - 18 commits from branch wine:master
    • 6efc6ccd - vkd3d-shader: Add compiler option to specify matrix majority.
    • 066b8349 - tests: Add test file syntax to specify compiler options.
    • e2ebda0a - tests: Add some tests for matrix packing compiler options.

    Compare with previous version

    • Could we add vkd3d-compiler support as well, please?

      I notice we're missing vkd3d-compiler support for VKD3D_SHADER_COMPILE_OPTION_TYPED_UAV and VKD3D_SHADER_COMPILE_OPTION_WRITE_TESS_GEOM_POINT_SIZE as well; it would be even better if we could add those as well.

    • Author Developer

      Sure. Pushed something for matrix order option. I'll add missing options later in a separate MR.

    • Please register or sign in to reply
  • Nikolay Sivov added 4 commits

    added 4 commits

    • e738f9c5 - vkd3d-shader: Add compiler option to specify matrix majority.
    • a5d5640b - tests: Add test file syntax to specify compiler options.
    • 0215446c - tests: Add some tests for matrix packing compiler options.
    • 04c1849c - vkd3d-compiler: Add CLI option to specify default matrix packing order.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Giovanni Mascellani approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

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