[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."
From: Conor McCarthy cmccarthy@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;
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?
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?
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 zwAnd 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.
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.
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)