[PATCH 0/2] MR381: vkd3d-shader/tpf: Handle "COLOR" semantic the same way as "sv_target" in the parser.
From: Nikolay Sivov nsivov@codeweavers.com
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/tpf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index c79b8057..ba9e0f5a 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2512,8 +2512,10 @@ static bool shader_sm4_init(struct vkd3d_shader_sm4_parser *sm4, const uint32_t struct signature_element *e = &output_signature->elements[i];
if (version.type == VKD3D_SHADER_TYPE_PIXEL - && ascii_strcasecmp(e->semantic_name, "SV_Target")) + && ascii_strcasecmp(e->semantic_name, "SV_Target") + && ascii_strcasecmp(e->semantic_name, "COLOR")) continue; + if (e->register_index >= ARRAY_SIZE(sm4->output_map)) { WARN("Invalid output index %u.\n", e->register_index);
From: Nikolay Sivov nsivov@codeweavers.com
The write_sm4_signature() helper exlicitly handles "color", "depth", "position" semantics and turns them to corresponding system values names. This does not currently work because usage is not returned for said names.
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com --- libs/vkd3d-shader/tpf.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index ba9e0f5a..917621db 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -2786,8 +2786,7 @@ bool hlsl_sm4_usage_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_semant { if (!ascii_strcasecmp(semantic->name, semantics[i].name) && output == semantics[i].output - && ctx->profile->type == semantics[i].shader_type - && !ascii_strncasecmp(semantic->name, "sv_", 3)) + && ctx->profile->type == semantics[i].shader_type) { *usage = semantics[i].usage; return true;
Francisco Casas (@fcasas) commented about libs/vkd3d-shader/tpf.c:
{ if (!ascii_strcasecmp(semantic->name, semantics[i].name) && output == semantics[i].output
&& ctx->profile->type == semantics[i].shader_type&& !ascii_strncasecmp(semantic->name, "sv_", 3))
This makes me wonder about the reason for this check to exist in the first place. It being there means that the entries in `semantics[]` that don't start with "sv_" have no effect, since 464dae2c.
Francisco Casas (@fcasas) commented about libs/vkd3d-shader/tpf.c:
struct signature_element *e = &output_signature->elements[i]; if (version.type == VKD3D_SHADER_TYPE_PIXEL
&& ascii_strcasecmp(e->semantic_name, "SV_Target"))
&& ascii_strcasecmp(e->semantic_name, "SV_Target")&& ascii_strcasecmp(e->semantic_name, "COLOR"))
Ok, took me a while to find out the implications of this.
So, this for loop has the purpose of initializing sm4->output_map[] when reading dxbc-tpf binaries.
And this array is used to translate, for pixel shaders, references to the output registers (VKD3DSPR_OUTPUT) to the VKD3DSPR_COLOROUT registers, using the semantic_index as offset, in map_register().
So, if we find a say, "COLOR2" semantic in the output signature of a SM4 pixel shader binary we would be correctly mapping the arbitrary output register to VKD3DSPR_COLOROUT2 after reading the shader.
I am not sure this change is necessary.
My understanding is that the "COLOR" semantic is invalid as a ps_4_0 output semantic, except when compatibility mode is used, and in this case it is written as "SV_Target", so no dxbc-tpf **pixel shader** binary should have "COLOR" as a semantic on its output signature.
On Fri Sep 29 16:09:25 2023 +0000, Francisco Casas wrote:
Ok, took me a while to find out the implications of this. So, this for loop has the purpose of initializing sm4->output_map[] when reading dxbc-tpf binaries. And this array is used to translate, for pixel shaders, references to the output registers (VKD3DSPR_OUTPUT) to the VKD3DSPR_COLOROUT registers, using the semantic_index as offset, in map_register(). So, if we find a say, "COLOR2" semantic in the output signature of a SM4 pixel shader binary we would be correctly mapping the arbitrary output register to VKD3DSPR_COLOROUT2 after reading the shader. I am not sure this change is necessary. My understanding is that the "COLOR" semantic is invalid as a ps_4_0 output semantic, except when compatibility mode is used, and in this case it is written as "SV_Target", so no dxbc-tpf **pixel shader** binary should have "COLOR" as a semantic on its output signature.
That's what second commit does. Regarding it being invalid, such binary is accepted on windows for disassembling at least. I don't know if it makes sense for the runtime itself. It's not that important by itself, once such semantics are mapped to correct system values, so we can of course drop it.
That's what second commit does.
Ah, I see, indeed 2/2 makes us write "SV_Target" instead of "color" in the output signature as the native compiler does when passing "-Gec".
Regarding it being invalid, such binary is accepted on windows for disassembling at least. I don't know if it makes sense for the runtime itself. It's not that important by itself, once such semantics are mapped to correct system values, so we can of course drop it.
Right. I also don't think it is important to replicate native's disassembler behavior for these pixel shaders with invalid signatures.
But now that I think about it, 1/2 could have the benefit of allowing the vkd3d to run shaders that were compiled wrong by the current and previous versions of vkd3d-shader.
Still, I am not sure adding this exception for "retro-compatibility" with a wrong behavior is desired. I would be more for printing a warning encompassing all invalid output semantics rather than just handling "COLOR":
```c if (version.type == VKD3D_SHADER_TYPE_PIXEL && ascii_strcasecmp(e->semantic_name, "SV_Target")) { WARN("Unexpected output semantic for pixel shader %s.\n", debugstr_a(e->semantic_name)); continue; } ```
Btw, we should add the checks for invalid output semantics at compilation time at some point.
On Fri Sep 29 17:21:01 2023 +0000, Francisco Casas wrote:
That's what second commit does.
Ah, I see, indeed 2/2 makes us write "SV_Target" instead of "color" in the output signature as the native compiler does when passing "-Gec".
Regarding it being invalid, such binary is accepted on windows for
disassembling at least. I don't know if it makes sense for the runtime itself. It's not that important by itself, once such semantics are mapped to correct system values, so we can of course drop it. Right. I also don't think it is important to replicate native's disassembler behavior for these pixel shaders with invalid signatures. But now that I think about it, 1/2 could have the benefit of allowing the vkd3d to run shaders that were compiled wrong by the current and previous versions of vkd3d-shader. Still, I am not sure adding this exception for "retro-compatibility" with a wrong behavior is desired. I would be more for printing a warning encompassing all invalid output semantics rather than just handling "COLOR":
if (version.type == VKD3D_SHADER_TYPE_PIXEL && ascii_strcasecmp(e->semantic_name, "SV_Target")) { WARN("Unexpected output semantic for pixel shader %s.\n", debugstr_a(e->semantic_name)); continue; }Btw, we should add the checks for invalid output semantics at compilation time at some point.
Right, fixing 2/2 is more important. I'd be happy to drop 1/2 once 2/2 is approved.
Minor, but since I noticed it: 2/2 has a typo in the commit message, "exlicitly" instead of "explicitly".
Incidentally, mapping outputs in shader_sm4_read_param() (or the parser in general) is historic, and probably not the ideal place these days. Note that we map these back to "o" registers in shader_dump_register() again; the more reasonable thing to do would probably be to do this mapping before/while generating output that needs it, but as far as I can tell we don't currently have any. I.e., we could probably just get rid of the mapping code.
Note that this MR causes CI test failures in tests/vkd3d_shader_api.
On Fri Sep 29 16:08:32 2023 +0000, Francisco Casas wrote:
This makes me wonder about the reason for this check to exist in the first place. It being there means that the entries in `semantics[]` that don't start with "sv_" have no effect, since 464dae2c.
It was probably an error on my part, I can only assume I mentally transferred the following check for sv_ inside the loop.
Did you actually find a native shader that has "color" in the output signature? According to my tests native will actually output "SV_Target" in this case.
That probably implies we should do the same, arguably even fixing that bug before we apply 2/2.
That probably implies we should do the same, arguably even fixing that bug before we apply 2/2.
Oh, never mind, we already have code to handle that. Sorry for the noise.
But yeah, question remains, do we actually want 1/2?
On Wed Oct 4 19:59:23 2023 +0000, Zebediah Figura wrote:
Did you actually find a native shader that has "color" in the output signature? According to my tests native will actually output "SV_Target" in this case. That probably implies we should do the same, arguably even fixing that bug before we apply 2/2.
No, I didn't find a native shader like that. On Windows it's always using sv_target, like you said. 2/2 is exactly to fix that. First patch 1/2 is to be able to dump broken output we currently create. Because it does not fail to disassemble on Windows.
On Wed Oct 4 20:00:09 2023 +0000, Zebediah Figura wrote:
That probably implies we should do the same, arguably even fixing that
bug before we apply 2/2. Oh, never mind, we already have code to handle that. Sorry for the noise. But yeah, question remains, do we actually want 1/2?
We don't necessarily want or need it, no. I can remove it, if that's preferred.
On Wed Oct 4 20:00:09 2023 +0000, Nikolay Sivov wrote:
We don't necessarily want or need it, no. I can remove it, if that's preferred.
Yes, I think we should remove 1/2 until we have evidence it's necessary.
-
Francisco Casas (＠fcasas) -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)