vkd3d-shader/hlsl: Implement sincos.
Another intrinsic we are missing.
I implemented this as just doing a sin and cos operation. Flattening this to one sincos
opcode should probably be done in our folding/optimization steps and is probably orthogonal to this MR.
Native behavior does not return very precise results, which is why the ulps are so big on the tests.
Merge request reports
Activity
/cc @fcasas
Is the HLSL definition saving us that much work?
I implemented this as just doing a sin and cos operation. Flattening this to one
sincos
opcode should probably be done in our folding/optimization steps and is probably orthogonal to this MR.It can, and I'm not opposed to accepting this merge request as-is, but if our IR is going to be capable of expressing sincos (and I think it should be) then I don't think I see a reason to emit the worse case at parse time.
Implementing it by emitting actual sincos instructions looks like it's going to be fairly complicated. Also, the official documentation and fxc output differ...
According to the docs, the sincos instruction works differently on sm1-3 and sm4+, and in both cases it will only do one float at a time. However, fxc will happily output
sincos sin_dst, cos_dst, src
. Just writing the SINCOS instruction for the respective shader model doesn't work (and outputs interesting things like all zeroes on sm4+, or NaNs on sm1-3.)I'd rather have the sincos intrinsic work at all rather than have it work optimally. As-is, this will output correct results, which I suppose is the important part.
You could argue that output in this case is the instruction stream, so correctness requirements apply to it as well. Were you able to make it use sincos instruction with with sm1-3? For that simply produces polynomial expressions. For sm4+ it is document to be per-component, and that's what it does, so I don't see a contradiction there.
IIRC native emits sincos in sm1 when operating on a single component, and uses polynomial expansion when operating on multiple components.
I'm not seeing anything in [1] that says it can only take a single component in sm4?
In order to implement sincos I believe we'll need a dedicated instr type, since nothing currently is capable of expressing multiple destinations.
Anyway it's not a huge concern, no, and like I said I'm not opposed to accepting this merge request as-is, but when we do add sincos to the IR, relying on CSE to combine a sin and cos seems worse than just emitting sincos up front.
[1] https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/sincos--sm4---asm-
I was looking here: https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/sincos---ps but this documentation applies only for SM up to 3. (I misread it.)
Thanks for the pointers, I can get on that.
added 8 commits
-
15e75707...32dc7ff4 - 7 commits from branch
wine:master
- 8bd1559e - vkd3d-shader/hlsl: Implement sincos.
-
15e75707...32dc7ff4 - 7 commits from branch
added 3 commits
-
8bd1559e...f318e565 - 2 commits from branch
wine:master
- 3c5e6a71 - vkd3d-shader/hlsl: Implement sincos.
-
8bd1559e...f318e565 - 2 commits from branch
Rebased.
For what it's worth, chances of this going in before the release are pretty much zero at this point; the last chance would have been about 2 weeks ago. That shouldn't prevent you from improving the MR and getting approvals from Elizabeth and Francisco, so that this is ready to be committed as soon as we do release, but there's little value in plain rebases right now.
added 19 commits
-
3c5e6a71...3d8fc1a4 - 18 commits from branch
wine:master
- 3cabdebb - vkd3d-shader/hlsl: Implement sincos.
-
3c5e6a71...3d8fc1a4 - 18 commits from branch
added 11 commits
-
3cabdebb...384810b4 - 10 commits from branch
wine:master
- 855b9713 - vkd3d-shader/hlsl: Implement the sincos() intrinsic.
-
3cabdebb...384810b4 - 10 commits from branch