Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Support rsqrt() and distance() intrinsics.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:hlsl_rsqrt into master
3 unresolved threads

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
10 10 uniform 0 float4 1.0 9.0 32.3 46.5
11 11 draw quad
12 12 probe all rgba (1.0, 3.0, 5.683309, 6.819091) 1
13
14 [pixel shader]
15 uniform float4 f;
16
17 float4 main() : sv_target
18 {
19 return rsqrt(f);
20 }
21
22 [test]
23 uniform 0 float4 1.0 9.0 4.0 16.0
24 draw quad
25 probe all rgba (1.0, 0.33333333, 0.5, 0.25) 1
  • Comment on lines +23 to +25

    It wouldn't be bad to test on negative numbers as well. That would mostly be a test for shader execution, rather than shader compilation, but it's still useful. Also, eventually we'll add constant folding to square root too, and it would be useful to know what is supposed to happen.

    BTW, that applies to sqrt() as well.

  • Author Developer

    As I understand, the only reason we test by execution is because matching compiler output exactly is not really required, or needs a lot of work and time, that's better spent elsewhere. For constants that applies to all other tests as well.

  • Those .shader_test files are meant to test both the compiler and the executor. So it's useful to have something that tests what happen to sqrt(-1), in order to ensure that to conversion to SPIR-V and then the execution through a Vulkan driver does what the shader expects.

  • What Giovanni said, although it's kind of a mix of both reasons. We don't want to match native compiler output exactly, but it's also useful to test the lower level compilers (for our purposes, dxbc -> spirv/glsl, but Mesa has also expressed interest in running our tests.)

  • Author Developer

    At least for rsqrt() results depend on shader model, for d3d9 input is going through abs() equivalent when executed, so it's never invalid. For d3d11 output contains NaNs. Detecting that with isnan() won't work, because that's not compiled reliably. We could probably use some inline isnan() logic, but still there is no way currently to pick different test result. I understand the importance of getting a good coverage, but that goes beyond implementing builtin functions. I don't think one that should block another.

  • I don't think it should block this patch, no.

  • I am not sure I understand the problem. As you said, you can reimplement isnan() with something like:

    bool4 xisnan(float4 x)
    {
        return (asuint(x) & 0x7FFFFFFF) > 0x7F800000;
    }

    (adapted from this tweet and this shader playground mentioned by Zeb)

    Or you can just pass the NaN to asuint(), so we can also test the actual NaN bits.

    Also, you can constrain the test to SM4 using:

    [require]
    shader model >= 4.0

    on the test with negative numbers.

    It's not important if we still do not support asuint() yet. You can leave the test todo, but at least have it pinpoint the expected outcome.

    This doesn't seem to be particularly difficult or long. Or am I missing something?

  • Author Developer

    I'd be happy to look into that, if this is the approach we want to take. It's more about improving existing testing tools, and not the problem this MR is for.

    I don't know if "require" ends up a correct name for this functionality, shader does not require specific version, results do. Limiting to >= 4.0 is also insufficient, because that does not show the difference with SM3 and below, which takes absolute value of the input internally.

  • I think it'd be better to test that the render target contains inf/nan, unless that differs or is impossible for some reason. I have tests in my local tree that do that for sm4 at least.

    I don't know if "require" ends up a correct name for this functionality, shader does not require specific version, results do. Limiting to >= 4.0 is also insufficient, because that does not show the difference with SM3 and below, which takes absolute value of the input internally.

    Eh, yeah, although [require] can be put anywhere, e.g. before the [test] block. It's maybe not the best name, but it doesn't seem awful; the point is just to restrict the platforms we run those tests on, for whatever reason.

  • Author Developer

    Yes, having actual values in expected results is better, if it's reliable. Require could mean "restrict to", so it's not bad. I can probably add support for "< 4.0" if that does not work already, and use that for d3d9.

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

    added 1 commit

    • 4e3a8373 - vkd3d-shader/hlsl: Support distance() intrinsic.

    Compare with previous version

  • Francisco Casas
    Francisco Casas @fcasas started a thread on an outdated change in commit 4e3a8373
  • 2485 2485 return !!add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_ADD, mul2, mul1_neg, loc);
    2486 2486 }
    2487 2487
    2488 static bool intrinsic_distance(struct hlsl_ctx *ctx,
    2489 const struct parse_initializer *params, const struct vkd3d_shader_location *loc)
    2490 {
    2491 struct hlsl_ir_node *neg, *add, *dot;
    2492
    2493 if (!elementwise_intrinsic_float_convert_args(ctx, params, loc))
    2494 return false;
    • Comment on lines +2493 to +2494

      distance() seems to not be subjected to the type conversion rules of element-wise intrinsics (which makes sense, because it isn't one). For instance, using distance(a, b) with a : float4x2 and b : float2x4 isn't allowed.

      It probably makes sense to remove these lines because the add_binary_arithmetic_expr() call should be taking care of the right type checks and conversions. Still, I think it would be good to add tests for some combinations of matrices, vectors, and scalar, to check that this is indeed the case.

      Edited by Francisco Casas
    • Nikolay Sivov changed this line in version 4 of the diff

      changed this line in version 4 of the diff

    • Author Developer

      Thanks, I removed conversion part.

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

    added 6 commits

    Compare with previous version

  • Nikolay Sivov added 1 commit

    added 1 commit

    • 161063f5 - vkd3d-shader/hlsl: Support distance() intrinsic.

    Compare with previous version

  • The distance() test is failing for me with d3d11 + Wine. It looks like a mesa bug; I'm looking into it.

  • Well, for one, 8.3666 is wrong, the result should be 7.48398304. I'm getting intermittent failures with even very simple shaders, though, which is very weird.

  • I'm annoyed at how long it took me to notice this, but the second line is "uniform 1", not "uniform 4", so that's where the uninitialized values are coming from.

  • Author Developer

    Oh, sorry about that. I didn't know it's counted in floats.

  • Nikolay Sivov added 6 commits

    added 6 commits

    Compare with previous version

  • Giovanni Mascellani mentioned in merge request !80 (merged)

    mentioned in merge request !80 (merged)

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Nikolay Sivov added 17 commits

    added 17 commits

    • 2ed69b05...902ddee5 - 15 commits from branch wine:master
    • ddc731a7 - vkd3d-shader/hlsl: Support rsqrt() intrinsic.
    • 0ad0ceb6 - vkd3d-shader/hlsl: Support distance() intrinsic.

    Compare with previous version

    • Currently, distance() fails when passing int parameters:

      int a, b;
      
      float4 main() : sv_target
      {
          return distance(a, b);
      }
      vkd3d/libs/vkd3d-shader/hlsl_sm4.c:1800: write_sm4_expr: Assertion `type_is_float(dst_type)' failed.
      Aborted (core dumped)

      it is worth noting that add_binary_arithmetic_expr() results in a value of the common type (which is int if both arguments are int).

    • Author Developer

      Thanks, it should do explicit conversion apparently. I pushed just that. Tests results unfortunately differ again, between d3d9 and d3d10+.

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

    added 49 commits

    • 0ad0ceb6...5f904e50 - 47 commits from branch wine:master
    • 815095e9 - vkd3d-shader/hlsl: Support rsqrt() intrinsic.
    • 42e8d309 - vkd3d-shader/hlsl: Support distance() intrinsic.

    Compare with previous version

  • Francisco Casas approved this merge request

    approved this merge request

  • Nikolay Sivov added 26 commits

    added 26 commits

    • 42e8d309...6ccde9e8 - 24 commits from branch wine:master
    • d3d3b2f1 - vkd3d-shader/hlsl: Support rsqrt() intrinsic.
    • a5cb9b14 - vkd3d-shader/hlsl: Support distance() intrinsic.

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading