vkd3d-shader/hlsl: Support rsqrt() and distance() intrinsics.
Merge request reports
Activity
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. 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 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 testtodo
, but at least have it pinpoint the expected outcome.This doesn't seem to be particularly difficult or long. Or am I missing something?
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.
- Resolved by Nikolay Sivov
Also, please add some tests for
distance()
too. Ideally every intrinsic should have at least a basic smoke test.
added 1 commit
- 4e3a8373 - vkd3d-shader/hlsl: Support distance() intrinsic.
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, usingdistance(a, b)
witha : float4x2
andb : 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 changed this line in version 4 of the diff
added 6 commits
-
4e3a8373...68c232cf - 4 commits from branch
wine:master
- 82c5ff9d - vkd3d-shader/hlsl: Support rsqrt() intrinsic.
- 7b28929e - vkd3d-shader/hlsl: Support distance() intrinsic.
-
4e3a8373...68c232cf - 4 commits from branch
added 1 commit
- 161063f5 - vkd3d-shader/hlsl: Support distance() intrinsic.
added 6 commits
-
161063f5...552926cf - 4 commits from branch
wine:master
- 11155ed2 - vkd3d-shader/hlsl: Support rsqrt() intrinsic.
- 2ed69b05 - vkd3d-shader/hlsl: Support distance() intrinsic.
-
161063f5...552926cf - 4 commits from branch
mentioned in merge request !80 (merged)
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.
-
2ed69b05...902ddee5 - 15 commits from branch
Currently,
distance()
fails when passingint
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 isint
if both arguments areint
).
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.
-
0ad0ceb6...5f904e50 - 47 commits from branch
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.
-
42e8d309...6ccde9e8 - 24 commits from branch