[PATCH vkd3d 00/11] vkd3d-shader: Fix patch constant handling in hull shaders.
This series mainly changes the way patch constant outputs are generated in hull shaders to always use a private array variable. This is done to fix several issues in the current imlementation, and to avoid excessive complexity in dealing with patch constants which stem from DXBC's design: 1) Tessellation factors and regular patch constants share the same register space in DXBC and can both be dynamically or statically indexed. The current implementation only works correctly in case tessellation factors do not share registers with regular outputs, and does not support all indexing patterns, leading to invalid code being generated in some cases. 2) Join phases can read patch constants written by a fork phase, also with dynamic indexing. However, the index ranges used by the join phase are not known at the time the fork phase is compiled. With these changes, tessellation shaders work correctly in Shadow of the Tomb Raider, which previously contained invalid SPIR-V and would crash the game if enabled. Note that enabling the tessellation option in the game triggers a separate issue related to descriptor updates, which will have to be addressed separately.
Fixes an assertion when compiling shaders with more than four clip or cull distances. Output arrays are arrays of scalars, so shifting the write mask is not very meaningful. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 75a3a4f..4b93e01 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4536,7 +4536,7 @@ static void vkd3d_dxbc_compiler_emit_store_shader_output(struct vkd3d_dxbc_compi ptr_type_id, output_id, vkd3d_dxbc_compiler_get_constant_uint(compiler, index)); object_id = vkd3d_dxbc_compiler_emit_swizzle(compiler, val_id, write_mask, output_info->component_type, VKD3D_NO_SWIZZLE, VKD3DSP_WRITEMASK_0 << i); - vkd3d_dxbc_compiler_emit_store(compiler, chain_id, VKD3DSP_WRITEMASK_0 << index, + vkd3d_dxbc_compiler_emit_store(compiler, chain_id, VKD3DSP_WRITEMASK_0, output_info->component_type, SpvStorageClassOutput, VKD3DSP_WRITEMASK_0 << i, object_id); ++index; } -- 2.23.0
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
Otherwise, if a private variable is used for the given output, vkd3d_dxbc_compiler_emit_store_shader_output will write to the private variable again instead of the actual output, and some outputs may never be emitted. This is common in hull shaders. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 4b93e01..1598ae0 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4343,10 +4343,6 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler entry = rb_get(&compiler->symbol_table, ®_symbol); } } - else if ((entry = rb_get(&compiler->symbol_table, ®_symbol))) - { - id = RB_ENTRY_VALUE(entry, const struct vkd3d_symbol, entry)->id; - } else { if (builtin) @@ -4393,7 +4389,7 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler if (use_private_variable) storage_class = SpvStorageClassPrivate; - if (entry) + if ((entry = rb_get(&compiler->symbol_table, ®_symbol))) var_id = RB_ENTRY_VALUE(entry, const struct vkd3d_symbol, entry)->id; else if (!use_private_variable) var_id = id; -- 2.23.0
Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 1598ae0..9b8da57 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4381,10 +4381,10 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler vkd3d_spirv_build_op_decorate(builder, id, SpvDecorationPatch, NULL, 0); vkd3d_dxbc_compiler_decorate_xfb_output(compiler, id, output_component_count, signature_element); - } - compiler->output_info[signature_idx].id = id; - compiler->output_info[signature_idx].component_type = component_type; + compiler->output_info[signature_idx].id = id; + compiler->output_info[signature_idx].component_type = component_type; + } if (use_private_variable) storage_class = SpvStorageClassPrivate; -- 2.23.0
Hi Philip, This causes a crash in the vkd3d tests, after line 28451 in tests/d3d12.c, when combined with only the preceding patch or the complete set. libs/vkd3d-shader/spirv.c:1638: vkd3d_spirv_get_type_id: Assertion `component_type != VKD3D_TYPE_VOID' failed. On Thu, Oct 17, 2019 at 6:36 AM Philip Rebohle < philip.rebohle(a)tu-dortmund.de> wrote:
Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 1598ae0..9b8da57 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4381,10 +4381,10 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler vkd3d_spirv_build_op_decorate(builder, id, SpvDecorationPatch, NULL, 0);
vkd3d_dxbc_compiler_decorate_xfb_output(compiler, id, output_component_count, signature_element); - }
- compiler->output_info[signature_idx].id = id; - compiler->output_info[signature_idx].component_type = component_type; + compiler->output_info[signature_idx].id = id; + compiler->output_info[signature_idx].component_type = component_type; + }
if (use_private_variable) storage_class = SpvStorageClassPrivate; -- 2.23.0
Hi, looks like that happens because vkd3d_dxbc_compiler_emit_shader_signature_outputs sets the ID and array mask, but not the component type. I suppose there are two ways to fix this: - 1) define the component type at the time the clip/cull distances are generated (i think this should always be FLOAT anyway), or - 2) drop this patch. Any preferences? I'm fine with either. - Philip PS: Sorry that I didn't test that earlier; I can only run a subset of the vkd3d tests at a time since there seems to be a severe memory leak going on somewhere and I only have 16GB in my machine. Am 22.10.19 um 16:17 schrieb Conor McCarthy:
Hi Philip,
This causes a crash in the vkd3d tests, after line 28451 in tests/d3d12.c, when combined with only the preceding patch or the complete set. libs/vkd3d-shader/spirv.c:1638: vkd3d_spirv_get_type_id: Assertion `component_type != VKD3D_TYPE_VOID' failed.
On Thu, Oct 17, 2019 at 6:36 AM Philip Rebohle <philip.rebohle(a)tu-dortmund.de <mailto:philip.rebohle(a)tu-dortmund.de>> wrote:
Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de <mailto:philip.rebohle(a)tu-dortmund.de>> --- libs/vkd3d-shader/spirv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 1598ae0..9b8da57 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4381,10 +4381,10 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler vkd3d_spirv_build_op_decorate(builder, id, SpvDecorationPatch, NULL, 0);
vkd3d_dxbc_compiler_decorate_xfb_output(compiler, id, output_component_count, signature_element); - }
- compiler->output_info[signature_idx].id = id; - compiler->output_info[signature_idx].component_type = component_type; + compiler->output_info[signature_idx].id = id; + compiler->output_info[signature_idx].component_type = component_type; + }
if (use_private_variable) storage_class = SpvStorageClassPrivate; -- 2.23.0
On Tue, 22 Oct 2019 at 18:06, Philip Rebohle <philip.rebohle(a)tu-dortmund.de> wrote:
looks like that happens because vkd3d_dxbc_compiler_emit_shader_signature_outputs sets the ID and array mask, but not the component type.
I suppose there are two ways to fix this: - 1) define the component type at the time the clip/cull distances are generated (i think this should always be FLOAT anyway), or - 2) drop this patch.
Any preferences? I'm fine with either.
I'm tending towards the first option. Henri
PS: Sorry that I didn't test that earlier; I can only run a subset of the vkd3d tests at a time since there seems to be a severe memory leak going on somewhere and I only have 16GB in my machine.
That doesn't seem right, 16 GiB should be plenty for running the vkd3d tests; it certainly works on the 16 GiB Intel SKL laptop I have here. Hopefully that's not a vkd3d issue, but if it is, it would be good to track that down.
On Wed, Oct 23, 2019 at 12:35 AM Philip Rebohle < philip.rebohle(a)tu-dortmund.de> wrote:
PS: Sorry that I didn't test that earlier; I can only run a subset of the vkd3d tests at a time since there seems to be a severe memory leak going on somewhere and I only have 16GB in my machine.
I had no trouble with my old 8 Gb machine, and I only see a small amount
of memory used while running the tests. What hardware and driver do you have? Conor
Hello, looks like this might have been a temporary driver issue on my end, I updated my Mesa build and now it works fine. FWIW I'm on an RX 480 using RADV (ACO branch). - Philip Am 23.10.19 um 14:09 schrieb Conor McCarthy:
On Wed, Oct 23, 2019 at 12:35 AM Philip Rebohle <philip.rebohle(a)tu-dortmund.de <mailto:philip.rebohle(a)tu-dortmund.de>> wrote:
PS: Sorry that I didn't test that earlier; I can only run a subset of the vkd3d tests at a time since there seems to be a severe memory leak going on somewhere and I only have 16GB in my machine.
I had no trouble with my old 8 Gb machine, and I only see a small amount of memory used while running the tests. What hardware and driver do you have?
Conor
Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- v2: Fix assertion with clip/cull distances. Supersedes patch 171404. --- libs/vkd3d-shader/spirv.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 1598ae0..8e3b53a 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4220,11 +4220,13 @@ static void vkd3d_dxbc_compiler_emit_shader_signature_outputs(struct vkd3d_dxbc_ { case VKD3D_SV_CLIP_DISTANCE: compiler->output_info[i].id = clip_distance_id; + compiler->output_info[i].component_type = VKD3D_TYPE_FLOAT; compiler->output_info[i].array_element_mask = clip_distance_mask; break; case VKD3D_SV_CULL_DISTANCE: compiler->output_info[i].id = cull_distance_id; + compiler->output_info[i].component_type = VKD3D_TYPE_FLOAT; compiler->output_info[i].array_element_mask = cull_distance_mask; break; @@ -4381,10 +4383,10 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler vkd3d_spirv_build_op_decorate(builder, id, SpvDecorationPatch, NULL, 0); vkd3d_dxbc_compiler_decorate_xfb_output(compiler, id, output_component_count, signature_element); - } - compiler->output_info[signature_idx].id = id; - compiler->output_info[signature_idx].component_type = component_type; + compiler->output_info[signature_idx].id = id; + compiler->output_info[signature_idx].component_type = component_type; + } if (use_private_variable) storage_class = SpvStorageClassPrivate; -- 2.23.0
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
Needed to support dynamically indexed output arrays. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 9b8da57..c83d05f 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2068,6 +2068,7 @@ struct vkd3d_dxbc_compiler uint32_t array_element_mask; } *output_info; uint32_t private_output_variable[MAX_REG_OUTPUT + 1]; /* 1 entry for oDepth */ + uint32_t private_output_variable_array_idx[MAX_REG_OUTPUT + 1]; /* 1 entry for oDepth */ uint32_t private_output_variable_write_mask[MAX_REG_OUTPUT + 1]; /* 1 entry for oDepth */ uint32_t epilogue_function_id; @@ -4552,6 +4553,7 @@ static void vkd3d_dxbc_compiler_emit_shader_epilogue_function(struct vkd3d_dxbc_ STATIC_ASSERT(ARRAY_SIZE(compiler->private_output_variable) == ARRAY_SIZE(param_id)); STATIC_ASSERT(ARRAY_SIZE(compiler->private_output_variable) == ARRAY_SIZE(param_type_id)); + STATIC_ASSERT(ARRAY_SIZE(compiler->private_output_variable) == ARRAY_SIZE(compiler->private_output_variable_array_idx)); STATIC_ASSERT(ARRAY_SIZE(compiler->private_output_variable) == ARRAY_SIZE(compiler->private_output_variable_write_mask)); phase = vkd3d_dxbc_compiler_get_current_shader_phase(compiler); @@ -4608,6 +4610,7 @@ static void vkd3d_dxbc_compiler_emit_shader_epilogue_function(struct vkd3d_dxbc_ vkd3d_spirv_build_op_function_end(builder); memset(compiler->private_output_variable, 0, sizeof(compiler->private_output_variable)); + memset(compiler->private_output_variable_array_idx, 0, sizeof(compiler->private_output_variable_array_idx)); memset(compiler->private_output_variable_write_mask, 0, sizeof(compiler->private_output_variable_write_mask)); compiler->epilogue_function_id = 0; } @@ -5555,6 +5558,7 @@ static void vkd3d_dxbc_compiler_leave_shader_phase(struct vkd3d_dxbc_compiler *c * Control point phase has separate output registers. */ memset(compiler->output_info, 0, signature->element_count * sizeof(*compiler->output_info)); memset(compiler->private_output_variable, 0, sizeof(compiler->private_output_variable)); + memset(compiler->private_output_variable_array_idx, 0, sizeof(compiler->private_output_variable_array_idx)); memset(compiler->private_output_variable_write_mask, 0, sizeof(compiler->private_output_variable_write_mask)); for (i = 0; i < signature->element_count; ++i) @@ -6455,16 +6459,28 @@ static uint32_t vkd3d_dxbc_compiler_emit_conditional_branch(struct vkd3d_dxbc_co static void vkd3d_dxbc_compiler_emit_shader_epilogue_invocation(struct vkd3d_dxbc_compiler *compiler) { struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; - uint32_t void_id, function_id, arguments[MAX_REG_OUTPUT]; + uint32_t void_id, type_id, ptr_type_id, function_id; + uint32_t arguments[MAX_REG_OUTPUT]; unsigned int i, count; if ((function_id = compiler->epilogue_function_id)) { void_id = vkd3d_spirv_get_op_type_void(builder); + type_id = vkd3d_spirv_get_type_id(builder, VKD3D_TYPE_FLOAT, 4); + ptr_type_id = vkd3d_spirv_get_op_type_pointer(builder, SpvStorageClassPrivate, type_id); for (i = 0, count = 0; i < ARRAY_SIZE(compiler->private_output_variable); ++i) { if (compiler->private_output_variable[i]) - arguments[count++] = compiler->private_output_variable[i]; + { + unsigned int argument_idx = count++; + arguments[argument_idx] = compiler->private_output_variable[i]; + + if (compiler->private_output_variable_array_idx[i]) + { + arguments[argument_idx] = vkd3d_spirv_build_op_access_chain1(builder, ptr_type_id, + arguments[argument_idx], compiler->private_output_variable_array_idx[i]); + } + } } vkd3d_spirv_build_op_function_call(builder, void_id, function_id, arguments, count); -- 2.23.0
Fork and join phases in hulll shaders allow dynamic indexing for all output registers, not just the tessellation factor built-ins. Moreover, the patch constant output register space is shared with join phases, which can read back the outputs computed in the fork phases, also allowing dynamic indexing. In order to support this in a not overly complex way, use a private array representing the entire patch constant space, and use epilogue functions to assign them to the actual output variables. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 40 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index c83d05f..76df8fc 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -2029,6 +2029,7 @@ struct vkd3d_hull_shader_variables { uint32_t tess_level_outer_id; uint32_t tess_level_inner_id; + uint32_t patch_constants_id; }; struct vkd3d_dxbc_compiler @@ -4322,7 +4323,8 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler write_mask = dst->write_mask; else if (get_shader_output_swizzle(compiler, signature_element->register_index) != VKD3D_NO_SWIZZLE || needs_private_io_variable(shader_signature, signature_element->register_index, - builtin, &output_component_count, &write_mask)) + builtin, &output_component_count, &write_mask) + || is_patch_constant) { use_private_variable = true; write_mask = VKD3DSP_WRITEMASK_ALL; @@ -4394,6 +4396,8 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler var_id = RB_ENTRY_VALUE(entry, const struct vkd3d_symbol, entry)->id; else if (!use_private_variable) var_id = id; + else if (is_patch_constant) + var_id = compiler->hs.patch_constants_id; else var_id = vkd3d_dxbc_compiler_emit_variable(compiler, &builder->global_stream, storage_class, VKD3D_TYPE_FLOAT, VKD3D_VEC4_SIZE); @@ -4401,15 +4405,19 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler { vkd3d_symbol_set_register_info(®_symbol, var_id, storage_class, use_private_variable ? VKD3D_TYPE_FLOAT : component_type, write_mask); - reg_symbol.info.reg.is_aggregate = use_private_variable ? false : array_size; + reg_symbol.info.reg.is_aggregate = use_private_variable ? is_patch_constant : array_size; if (!use_private_variable && is_control_point_phase(phase)) { reg_symbol.info.reg.member_idx = vkd3d_dxbc_compiler_get_invocation_id(compiler); reg_symbol.info.reg.is_dynamically_indexed = true; } + else if (is_patch_constant) + reg_symbol.info.reg.member_idx = reg->idx[0].offset; + vkd3d_dxbc_compiler_put_symbol(compiler, ®_symbol); - vkd3d_dxbc_compiler_emit_register_debug_name(builder, var_id, reg); + if (!is_patch_constant) + vkd3d_dxbc_compiler_emit_register_debug_name(builder, var_id, reg); } if (use_private_variable) @@ -4417,6 +4425,10 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler unsigned int idx = vkd3d_dxbc_compiler_get_output_variable_index(compiler, reg->idx[0].offset); compiler->private_output_variable[idx] = var_id; compiler->private_output_variable_write_mask[idx] |= dst->write_mask; + if (is_patch_constant) { + compiler->private_output_variable_array_idx[idx] = vkd3d_dxbc_compiler_get_constant_uint( + compiler, reg->idx[0].offset); + } if (!compiler->epilogue_function_id) compiler->epilogue_function_id = vkd3d_spirv_alloc_id(builder); } @@ -4627,6 +4639,27 @@ static void vkd3d_dxbc_compiler_emit_hull_shader_builtins(struct vkd3d_dxbc_comp vkd3d_dxbc_compiler_emit_input_register(compiler, &dst); } +static void vkd3d_dxbc_compiler_emit_hull_shader_patch_constants(struct vkd3d_dxbc_compiler *compiler) +{ + const struct vkd3d_shader_signature *signature = compiler->patch_constant_signature; + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + unsigned int signature_idx; + uint32_t register_count; + + register_count = 0; + + for (signature_idx = 0; signature_idx < signature->element_count; signature_idx++) + register_count = max(register_count, signature->elements[signature_idx].register_index + 1); + + if (register_count) + { + compiler->hs.patch_constants_id = vkd3d_dxbc_compiler_emit_array_variable(compiler, + &builder->global_stream, SpvStorageClassPrivate, VKD3D_TYPE_FLOAT, VKD3D_VEC4_SIZE, + register_count); + vkd3d_spirv_build_op_name(builder, compiler->hs.patch_constants_id, "opc"); + } +} + static void vkd3d_dxbc_compiler_emit_initial_declarations(struct vkd3d_dxbc_compiler *compiler) { const struct vkd3d_shader_transform_feedback_info *xfb_info = compiler->xfb_info; @@ -4640,6 +4673,7 @@ static void vkd3d_dxbc_compiler_emit_initial_declarations(struct vkd3d_dxbc_comp case VKD3D_SHADER_TYPE_HULL: vkd3d_spirv_set_execution_model(builder, SpvExecutionModelTessellationControl); vkd3d_dxbc_compiler_emit_hull_shader_builtins(compiler); + vkd3d_dxbc_compiler_emit_hull_shader_patch_constants(compiler); break; case VKD3D_SHADER_TYPE_DOMAIN: vkd3d_spirv_set_execution_model(builder, SpvExecutionModelTessellationEvaluation); -- 2.23.0
Private variables are always vec4, so using a sparse write mask here will lead to invalid code being generated when accessing the variable. This is needed for the following commits. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 76df8fc..74b6e52 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4320,7 +4320,7 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler storage_class = SpvStorageClassOutput; if ((use_private_variable = builtin && builtin->spirv_array_size)) - write_mask = dst->write_mask; + write_mask = VKD3DSP_WRITEMASK_ALL; else if (get_shader_output_swizzle(compiler, signature_element->register_index) != VKD3D_NO_SWIZZLE || needs_private_io_variable(shader_signature, signature_element->register_index, builtin, &output_component_count, &write_mask) -- 2.23.0
Line tessellation factors use two different DXBC semantics that both map to the same SPIR-V built-in. In this case, we cannot rely on the semantic index. Needed for the following commits to not break line tessellation. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 74b6e52..af749c5 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4474,6 +4474,25 @@ static void vkd3d_dxbc_compiler_emit_shader_phase_output(struct vkd3d_dxbc_compi vkd3d_dxbc_compiler_put_symbol(compiler, ®_symbol); } +static uint32_t vkd3d_dxbc_compiler_get_output_array_index(struct vkd3d_dxbc_compiler *compiler, + const struct vkd3d_shader_signature_element *e) +{ + enum vkd3d_shader_input_sysval_semantic sysval; + const struct vkd3d_spirv_builtin *builtin; + + sysval = vkd3d_siv_from_sysval_indexed(e->sysval_semantic, e->semantic_index); + builtin = get_spirv_builtin_for_sysval(compiler, sysval); + + switch (sysval) + { + case VKD3D_SIV_LINE_DETAIL_TESS_FACTOR: + case VKD3D_SIV_LINE_DENSITY_TESS_FACTOR: + return builtin->member_idx; + default: + return e->semantic_index; + } +} + static void vkd3d_dxbc_compiler_emit_store_shader_output(struct vkd3d_dxbc_compiler *compiler, const struct vkd3d_shader_signature_element *output, const struct vkd3d_shader_output_info *output_info, @@ -4482,7 +4501,7 @@ static void vkd3d_dxbc_compiler_emit_store_shader_output(struct vkd3d_dxbc_compi unsigned int dst_write_mask, use_mask, uninit_mask, swizzle, mask; struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; uint32_t type_id, zero_id, ptr_type_id, chain_id, object_id; - unsigned int i, index; + unsigned int i, index, array_idx; uint32_t output_id; dst_write_mask = output->mask & 0xff; @@ -4535,7 +4554,8 @@ static void vkd3d_dxbc_compiler_emit_store_shader_output(struct vkd3d_dxbc_compi type_id = vkd3d_spirv_get_type_id(builder, output_info->component_type, 1); ptr_type_id = vkd3d_spirv_get_op_type_pointer(builder, SpvStorageClassOutput, type_id); mask = output_info->array_element_mask; - mask &= (1u << (output->semantic_index * VKD3D_VEC4_SIZE)) - 1; + array_idx = vkd3d_dxbc_compiler_get_output_array_index(compiler, output); + mask &= (1u << (array_idx * VKD3D_VEC4_SIZE)) - 1; for (i = 0, index = vkd3d_popcount(mask); i < VKD3D_VEC4_SIZE; ++i) { if (!(write_mask & (VKD3DSP_WRITEMASK_0 << i))) -- 2.23.0
Needed to accurately map tessellation factor semantics. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index af749c5..4dd25a5 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -129,7 +129,7 @@ static const void *vkd3d_find_struct_(const struct vkd3d_struct *chain, return NULL; } -static enum vkd3d_shader_input_sysval_semantic vkd3d_siv_from_sysval(enum vkd3d_sysval_semantic sysval) +static enum vkd3d_shader_input_sysval_semantic vkd3d_siv_from_sysval_indexed(enum vkd3d_sysval_semantic sysval, uint32_t index) { switch (sysval) { @@ -137,12 +137,29 @@ static enum vkd3d_shader_input_sysval_semantic vkd3d_siv_from_sysval(enum vkd3d_ return VKD3D_SIV_NONE; case VKD3D_SV_POSITION: return VKD3D_SIV_POSITION; + case VKD3D_SV_TESS_FACTOR_QUADEDGE: + return VKD3D_SIV_QUAD_U0_TESS_FACTOR + index; + case VKD3D_SV_TESS_FACTOR_QUADINT: + return VKD3D_SIV_QUAD_U_INNER_TESS_FACTOR + index; + case VKD3D_SV_TESS_FACTOR_TRIEDGE: + return VKD3D_SIV_TRIANGLE_U_TESS_FACTOR + index; + case VKD3D_SV_TESS_FACTOR_TRIINT: + return VKD3D_SIV_TRIANGLE_INNER_TESS_FACTOR; + case VKD3D_SV_TESS_FACTOR_LINEDET: + return VKD3D_SIV_LINE_DETAIL_TESS_FACTOR; + case VKD3D_SV_TESS_FACTOR_LINEDEN: + return VKD3D_SIV_LINE_DENSITY_TESS_FACTOR; default: - FIXME("Unhandled sysval %#x.\n", sysval); + FIXME("Unhandled sysval %#x, index %u.\n", sysval, index); return VKD3D_SIV_NONE; } } +static enum vkd3d_shader_input_sysval_semantic vkd3d_siv_from_sysval(enum vkd3d_sysval_semantic sysval) +{ + return vkd3d_siv_from_sysval_indexed(sysval, 0); +} + #define VKD3D_SPIRV_VERSION 0x00010000 #define VKD3D_SPIRV_GENERATOR_ID 18 #define VKD3D_SPIRV_GENERATOR_VERSION 1 -- 2.23.0
Will be used for shader phase outputs. Writing to the output info array during initialization would not work because it gets reset in each phase. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 4dd25a5..9bffc70 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4187,6 +4187,35 @@ static void calculate_clip_or_cull_distance_mask(const struct vkd3d_shader_signa *mask |= (e->mask & VKD3DSP_WRITEMASK_ALL) << (VKD3D_VEC4_SIZE * e->semantic_index); } +static uint32_t calculate_sysval_array_mask(struct vkd3d_dxbc_compiler *compiler, + const struct vkd3d_shader_signature *signature, enum vkd3d_shader_input_sysval_semantic sysval) +{ + const struct vkd3d_shader_signature_element *e; + const struct vkd3d_spirv_builtin *sig_builtin; + const struct vkd3d_spirv_builtin *builtin; + uint32_t signature_idx, mask = 0; + + builtin = get_spirv_builtin_for_sysval(compiler, sysval); + + if (!builtin) + { + FIXME("Unhandled sysval %#x.\n", sysval); + return 0; + } + + for (signature_idx = 0; signature_idx < signature->element_count; signature_idx++) + { + e = &signature->elements[signature_idx]; + + sig_builtin = get_spirv_builtin_for_sysval(compiler, vkd3d_siv_from_sysval_indexed(e->sysval_semantic, e->semantic_index)); + + if (sig_builtin && sig_builtin->spirv_builtin == builtin->spirv_builtin) + mask |= (e->mask & VKD3DSP_WRITEMASK_ALL) << (VKD3D_VEC4_SIZE * sig_builtin->member_idx); + } + + return mask; +} + /* Emits arrayed SPIR-V built-in variables. */ static void vkd3d_dxbc_compiler_emit_shader_signature_outputs(struct vkd3d_dxbc_compiler *compiler) { -- 2.23.0
Uses the private patch constant array for tess factor built-ins. Fixes two separate issues encountered in Shadow of the Tomb Raider: - The output registers that have one component mapped to any of the TESS_FACTOR sysvals can have their other components mapped to a regular patch constant output, in which case we need to use a private io variable. - The tess factor outputs are not necessarily dynamically indexed within shader code. Previously, this did not work correctly and lead to invalid store operations in the generated SPIR-V. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 84 ++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 46 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 9bffc70..7678db0 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -4311,6 +4311,31 @@ static void vkd3d_dxbc_compiler_emit_output_register(struct vkd3d_dxbc_compiler vkd3d_dxbc_compiler_emit_register_debug_name(builder, output_id, reg); } +static uint32_t vkd3d_dxbc_compiler_emit_shader_phase_builtin_variable(struct vkd3d_dxbc_compiler *compiler, + const struct vkd3d_shader_phase *phase, const struct vkd3d_spirv_builtin *builtin) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + uint32_t *variable_id, id; + + variable_id = NULL; + + if (builtin->spirv_builtin == SpvBuiltInTessLevelOuter) + variable_id = &compiler->hs.tess_level_outer_id; + else if (builtin->spirv_builtin == SpvBuiltInTessLevelInner) + variable_id = &compiler->hs.tess_level_inner_id; + + if (variable_id && *variable_id) + return *variable_id; + + id = vkd3d_dxbc_compiler_emit_builtin_variable(compiler, builtin, SpvStorageClassOutput, 0); + if (phase->type == VKD3DSIH_HS_FORK_PHASE || phase->type == VKD3DSIH_HS_JOIN_PHASE) + vkd3d_spirv_build_op_decorate(builder, id, SpvDecorationPatch, NULL, 0); + + if (variable_id) + *variable_id = id; + return id; +} + static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler, const struct vkd3d_shader_dst_param *dst, enum vkd3d_shader_input_sysval_semantic sysval) { @@ -4396,8 +4421,19 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler { if (builtin) { - id = vkd3d_dxbc_compiler_emit_builtin_variable(compiler, builtin, storage_class, array_size); + if (phase) + id = vkd3d_dxbc_compiler_emit_shader_phase_builtin_variable(compiler, phase, builtin); + else + id = vkd3d_dxbc_compiler_emit_builtin_variable(compiler, builtin, storage_class, array_size); + + if (builtin->spirv_array_size) + { + compiler->output_info[signature_idx].array_element_mask = + calculate_sysval_array_mask(compiler, shader_signature, sysval); + } + vkd3d_dxbc_compiler_emit_register_execution_mode(compiler, &dst->reg); + if (component_idx) FIXME("Unhandled component index %u.\n", component_idx); } @@ -4480,46 +4516,6 @@ static void vkd3d_dxbc_compiler_emit_output(struct vkd3d_dxbc_compiler *compiler } } -static void vkd3d_dxbc_compiler_emit_shader_phase_output(struct vkd3d_dxbc_compiler *compiler, - const struct vkd3d_shader_phase *phase, const struct vkd3d_shader_dst_param *dst, - enum vkd3d_shader_input_sysval_semantic sysval) -{ - struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; - const struct vkd3d_shader_register *reg = &dst->reg; - const struct vkd3d_spirv_builtin *builtin; - struct vkd3d_symbol reg_symbol; - uint32_t *variable_id; - - variable_id = NULL; - if ((builtin = get_spirv_builtin_for_sysval(compiler, sysval))) - { - if (builtin->spirv_builtin == SpvBuiltInTessLevelOuter) - variable_id = &compiler->hs.tess_level_outer_id; - else if (builtin->spirv_builtin == SpvBuiltInTessLevelInner) - variable_id = &compiler->hs.tess_level_inner_id; - } - - if (!variable_id) - { - FIXME("Unhandled shader phase output register %#x, sysval %#x.\n", reg->type, sysval); - return; - } - - if (!*variable_id) - { - *variable_id = vkd3d_dxbc_compiler_emit_builtin_variable(compiler, builtin, SpvStorageClassOutput, 0); - if (phase->type == VKD3DSIH_HS_FORK_PHASE || phase->type == VKD3DSIH_HS_JOIN_PHASE) - vkd3d_spirv_build_op_decorate(builder, *variable_id, SpvDecorationPatch, NULL, 0); - } - - vkd3d_symbol_make_register(®_symbol, reg); - vkd3d_symbol_set_register_info(®_symbol, *variable_id, SpvStorageClassOutput, - builtin->component_type, vkd3d_write_mask_from_component_count(builtin->component_count)); - reg_symbol.info.reg.member_idx = builtin->member_idx; - reg_symbol.info.reg.is_aggregate = builtin->spirv_array_size; - vkd3d_dxbc_compiler_put_symbol(compiler, ®_symbol); -} - static uint32_t vkd3d_dxbc_compiler_get_output_array_index(struct vkd3d_dxbc_compiler *compiler, const struct vkd3d_shader_signature_element *e) { @@ -5381,15 +5377,11 @@ static void vkd3d_dxbc_compiler_emit_dcl_output_siv(struct vkd3d_dxbc_compiler * { enum vkd3d_shader_input_sysval_semantic sysval; const struct vkd3d_shader_dst_param *dst; - const struct vkd3d_shader_phase *phase; dst = &instruction->declaration.register_semantic.reg; sysval = instruction->declaration.register_semantic.sysval_semantic; - if ((phase = vkd3d_dxbc_compiler_get_current_shader_phase(compiler))) - vkd3d_dxbc_compiler_emit_shader_phase_output(compiler, phase, dst, sysval); - else - vkd3d_dxbc_compiler_emit_output(compiler, dst, sysval); + vkd3d_dxbc_compiler_emit_output(compiler, dst, sysval); } static bool vkd3d_dxbc_compiler_check_index_range(struct vkd3d_dxbc_compiler *compiler, -- 2.23.0
This case needs special care since both VKD3DSPR_INPUT in the control point phase and VKD3DSPR_INCONTROLPOINT in fork/join phases refer to the same set of input variables, and we should not declare input variables with the same location twice. Encountered in Shadow of the Tomb Raider. Signed-off-by: Philip Rebohle <philip.rebohle(a)tu-dortmund.de> --- libs/vkd3d-shader/spirv.c | 42 +++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 7678db0..f5f1d6b 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -3916,6 +3916,7 @@ static uint32_t vkd3d_dxbc_compiler_emit_input(struct vkd3d_dxbc_compiler *compi enum vkd3d_component_type component_type; uint32_t val_id, input_id, var_id; struct vkd3d_symbol reg_symbol; + struct vkd3d_symbol tmp_symbol; SpvStorageClass storage_class; struct rb_entry *entry = NULL; bool use_private_var = false; @@ -4000,19 +4001,39 @@ static uint32_t vkd3d_dxbc_compiler_emit_input(struct vkd3d_dxbc_compiler *compi } else { - unsigned int location = reg_idx; + input_id = 0; - if (reg->type == VKD3DSPR_PATCHCONST) - location += compiler->input_signature->element_count; + if (compiler->shader_type == VKD3D_SHADER_TYPE_HULL && reg->type == VKD3DSPR_INCONTROLPOINT) + { + tmp_symbol = reg_symbol; + tmp_symbol.key.reg.type = VKD3DSPR_INPUT; - input_id = vkd3d_dxbc_compiler_emit_array_variable(compiler, &builder->global_stream, - storage_class, component_type, input_component_count, array_size); - vkd3d_spirv_add_iface_variable(builder, input_id); - vkd3d_spirv_build_op_decorate1(builder, input_id, SpvDecorationLocation, location); - if (component_idx) - vkd3d_spirv_build_op_decorate1(builder, input_id, SpvDecorationComponent, component_idx); + if ((entry = rb_get(&compiler->symbol_table, &tmp_symbol))) + { + tmp_symbol = *RB_ENTRY_VALUE(entry, const struct vkd3d_symbol, entry); + tmp_symbol.key.reg.type = VKD3DSPR_INCONTROLPOINT; + vkd3d_dxbc_compiler_put_symbol(compiler, &tmp_symbol); + + input_id = tmp_symbol.id; + } + } + + if (!entry) + { + unsigned int location = reg_idx; - vkd3d_dxbc_compiler_emit_interpolation_decorations(compiler, input_id, interpolation_mode); + if (reg->type == VKD3DSPR_PATCHCONST) + location += compiler->input_signature->element_count; + + input_id = vkd3d_dxbc_compiler_emit_array_variable(compiler, &builder->global_stream, + storage_class, component_type, input_component_count, array_size); + vkd3d_spirv_add_iface_variable(builder, input_id); + vkd3d_spirv_build_op_decorate1(builder, input_id, SpvDecorationLocation, location); + if (component_idx) + vkd3d_spirv_build_op_decorate1(builder, input_id, SpvDecorationComponent, component_idx); + + vkd3d_dxbc_compiler_emit_interpolation_decorations(compiler, input_id, interpolation_mode); + } } if (reg->type == VKD3DSPR_PATCHCONST) @@ -4121,6 +4142,7 @@ static void vkd3d_dxbc_compiler_emit_shader_phase_input(struct vkd3d_dxbc_compil switch (reg->type) { case VKD3DSPR_INPUT: + case VKD3DSPR_INCONTROLPOINT: vkd3d_dxbc_compiler_emit_input(compiler, dst, VKD3D_SIV_NONE, VKD3DSIM_NONE); return; case VKD3DSPR_PRIMID: -- 2.23.0
Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>
Hi Philip, Is the descriptor update issue expected to crash the game? Using these patches and tessellation I get a hard crash requiring reboot at the start of benchmark scene 2. Mesa 19.2.1 and AMD RX 580. On Thu, Oct 17, 2019 at 5:57 AM Philip Rebohle < philip.rebohle(a)tu-dortmund.de> wrote:
This series mainly changes the way patch constant outputs are generated in hull shaders to always use a private array variable. This is done to fix several issues in the current imlementation, and to avoid excessive complexity in dealing with patch constants which stem from DXBC's design:
1) Tessellation factors and regular patch constants share the same register space in DXBC and can both be dynamically or statically indexed. The current implementation only works correctly in case tessellation factors do not share registers with regular outputs, and does not support all indexing patterns, leading to invalid code being generated in some cases.
2) Join phases can read patch constants written by a fork phase, also with dynamic indexing. However, the index ranges used by the join phase are not known at the time the fork phase is compiled.
With these changes, tessellation shaders work correctly in Shadow of the Tomb Raider, which previously contained invalid SPIR-V and would crash the game if enabled.
Note that enabling the tessellation option in the game triggers a separate issue related to descriptor updates, which will have to be addressed separately.
Hi, unfortunately that is currently the case, yes, since some descriptors are left completely undefined. On RADV you can work around the system hang by setting RADV_DEBUG=zerovram, however you will see rendering artifacts. - Philip Am 18.10.19 um 15:59 schrieb Conor McCarthy:
Hi Philip,
Is the descriptor update issue expected to crash the game? Using these patches and tessellation I get a hard crash requiring reboot at the start of benchmark scene 2. Mesa 19.2.1 and AMD RX 580.
On Thu, Oct 17, 2019 at 5:57 AM Philip Rebohle <philip.rebohle(a)tu-dortmund.de <mailto:philip.rebohle(a)tu-dortmund.de>> wrote:
This series mainly changes the way patch constant outputs are generated in hull shaders to always use a private array variable. This is done to fix several issues in the current imlementation, and to avoid excessive complexity in dealing with patch constants which stem from DXBC's design:
1) Tessellation factors and regular patch constants share the same register space in DXBC and can both be dynamically or statically indexed. The current implementation only works correctly in case tessellation factors do not share registers with regular outputs, and does not support all indexing patterns, leading to invalid code being generated in some cases.
2) Join phases can read patch constants written by a fork phase, also with dynamic indexing. However, the index ranges used by the join phase are not known at the time the fork phase is compiled.
With these changes, tessellation shaders work correctly in Shadow of the Tomb Raider, which previously contained invalid SPIR-V and would crash the game if enabled.
Note that enabling the tessellation option in the game triggers a separate issue related to descriptor updates, which will have to be addressed separately.
participants (4)
-
Conor McCarthy -
Henri Verbeet -
Henri Verbeet -
Philip Rebohle