[PATCH v2 0/1] MR536: vkd3d-shader/ir: Revert "Do not merge signature elements which have different interpolation modes."
-- v2: vkd3d-shader/ir: Revert "Do not merge signature elements which have different interpolation modes." https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/536
From: Conor McCarthy <cmccarthy(a)codeweavers.com> This reverts commit b5c067b41a173e2ab252d5a3588f807c3ade5b2a. The commit causes regressions in other shaders, and the issue it fixed is currently not known to occur. --- libs/vkd3d-shader/ir.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/ir.c b/libs/vkd3d-shader/ir.c index d38b3c397..cbe07663d 100644 --- a/libs/vkd3d-shader/ir.c +++ b/libs/vkd3d-shader/ir.c @@ -780,9 +780,8 @@ static bool shader_signature_merge(struct shader_signature *s, uint8_t range_map f = &elements[j]; /* Merge different components of the same register unless sysvals are different, - * interpolation modes are different, or it will be relative-addressed. */ + * or it will be relative-addressed. */ if (f->register_index != e->register_index || f->sysval_semantic != e->sysval_semantic - || f->interpolation_mode != e->interpolation_mode || range_map_get_register_count(range_map, f->register_index, f->mask) > 1) break; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/536
So why is the original commit wrong/superfluous? Arguably this incident simply re-emphasises the need for test coverage for changes like these; if we're going to revert this, could we at least get a test this time? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/536#note_56696
On Fri Jan 5 07:00:22 2024 +0000, Henri Verbeet wrote:
So why is the original commit wrong/superfluous? Arguably this incident simply re-emphasises the need for test coverage for changes like these; if we're going to revert this, could we at least get a test this time? This issue is a little more specific than it first appeared. It occurs when an I/O row is split into multiple elements by components, but the first element is unused, e.g.:
``` Name Index Mask Register SysValue Format Used -------------------- ----- ------ -------- -------- ------- ------ sv_position 0 xyzw 0 POS float texcoord 0 xy 1 NONE float texcoord 1 zw 1 NONE float zw ``` And the declaration contains an interpolation mode: `dcl_input_ps constant v1.zw` The old fix needs to be reverted, and replaced with a new fix. When split elements are merged, the interpolation mode should be taken from those which have one. I was unable to make a shader runner test which touches the issue. My knowledge of interpolation modes and their effects isn't great. Any suggestions? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/536#note_56743
On Fri Jan 5 07:00:22 2024 +0000, Conor McCarthy wrote:
This issue is a little more specific than it first appeared. It occurs when an I/O row is split into multiple elements by components, but the first element is unused, e.g.: ``` Name Index Mask Register SysValue Format Used -------------------- ----- ------ -------- -------- ------- ------ sv_position 0 xyzw 0 POS float texcoord 0 xy 1 NONE float texcoord 1 zw 1 NONE float zw ``` And the declaration contains an interpolation mode: `dcl_input_ps constant v1.zw` The old fix needs to be reverted, and replaced with a new fix. When split elements are merged, the interpolation mode should be taken from those which have one. I was unable to make a shader runner test which touches the issue. My knowledge of interpolation modes and their effects isn't great. Any suggestions? I have a test which should work, but only with fxc. Our HLSL compiler doesn't split the VS outputs and PS inputs. A d3d12 test seems excessive, but the only way it will work in shader runner is if the compiler is patched so the signatures match fxc.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/536#note_56763
A d3d12 test seems excessive
I don't think we should keep a particularly high barrier on adding d3d12 tests. It would be nice to move as many as possible of them to the shader runner to save on boilerplate (and to make the shader runner language more powerful so that more tests can be converted), but in the meantime I don't think we want to give up on d3d12 tests because they're too many or because only some features can be tested there. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/536#note_56769
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)