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.
From: Jan Sikorski jsikorski@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
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?
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.
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.
Looks like the shader is shipped with the game. Windows accepts it and it renders nothing.
-
Conor McCarthy (＠cmccarthy) -
Henri Verbeet (＠hverbeet)
-
Jan Sikorski
-
Jan Sikorski (＠jsikorski)
-
Zebediah Figura (＠zfigura)