[PATCH 0/1] MR212: Always terminate main block.
I encountered a case where a game provides an empty fragment shader, one that disassembles to just: ``` ps_5_0 dcl_globalFlags refactoringAllowed ``` which gets translated to: ``` OpCapability Shader OpMemoryModel Logical GLSL450 OpEntryPoint Fragment %main "main" OpExecutionMode %main OriginUpperLeft OpName %main "main" %void = OpTypeVoid %3 = OpTypeFunction %void %main = OpFunction %void None %3 %4 = OpLabel OpFunctionEnd ``` This patch is to detect that the OpLabel has not been terminated by a corresponding top-level return, and add it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/212
From: Jan Sikorski <jsikorski(a)codeweavers.com> --- libs/vkd3d-shader/spirv.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index ed24e743..2e289e3e 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2247,6 +2247,7 @@ struct spirv_compiler struct vkd3d_push_constant_buffer_binding *push_constants; const struct vkd3d_shader_spirv_target_info *spirv_target_info; + bool terminated_main_block; bool after_declarations_section; struct shader_signature input_signature; struct shader_signature output_signature; @@ -7511,6 +7512,8 @@ static int spirv_compiler_emit_control_flow_instruction(struct spirv_compiler *c if (cf_info) cf_info->inside_block = false; + else + compiler->terminated_main_block = true; break; case VKD3DSIH_RETP: @@ -9471,6 +9474,9 @@ static int spirv_compiler_generate_spirv(struct spirv_compiler *compiler, if (result < 0) return result; + if (compiler->shader_type != VKD3D_SHADER_TYPE_HULL && !compiler->terminated_main_block) + vkd3d_spirv_build_op_return(builder); + if (!is_in_default_phase(compiler)) spirv_compiler_leave_shader_phase(compiler); else -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/212
We'll probably need something like this for d3dbc sources as well, since those generally aren't terminated by a "ret" instruction either, so in that sense this seems fine. I'm tempted to suggest handling this by appending a "ret" instruction to the vkd3d_shader_instruction_array if it doesn't already end with one, but I'd like to hear what Zeb and Conor think in any case. That shader does seem slightly suspicious though; do we know more about where it came from or how it was produced? Was this shipped with the application, or e.g. produced at run-time from HLSL source? Does it perhaps contain a RDEF section with a creator string? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/212#note_33821
We'll probably need something like this for d3dbc sources as well, since those generally aren't terminated by a "ret" instruction either, so in that sense this seems fine. I'm tempted to suggest handling this by appending a "ret" instruction to the vkd3d_shader_instruction_array if it doesn't already end with one, but I'd like to hear what Zeb and Conor think in any case.
I suppose the usual argument for preprocessing is that it'll help GLSL, but in this case GLSL doesn't actually need the ret, so it doesn't matter. I guess ultimately it doesn't seem to me like it matters enough to bikeshed anyway. FWIW, sm1 will need something like this, but that doesn't really affect anything. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/212#note_33835
Looks like something which should be fixed in the normaliser. But if the current design will be used, I suggest flipping the bool's meaning, i.e. initialise it to `true` if appending a RET may be necessary, and clear it when a RET is found. That would remove any need to change the DXIL patches to disable this fix. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/212#note_33880
Looks like the shader is shipped with the game. Windows accepts it and it renders nothing. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/212#note_33931
participants (5)
-
Conor McCarthy (＠cmccarthy) -
Henri Verbeet (＠hverbeet)
-
Jan Sikorski
-
Jan Sikorski (＠jsikorski)
-
Zebediah Figura (＠zfigura)