[PATCH vkd3d v2 6/7] vkd3d-shader: Handle extended instruction operand fields.
Prevents any non-zero value in fields starting at bit 14 from causing the modifier flags to be skipped, and emits proper warning and fixme messages. Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- Supersedes patch 211558. --- libs/vkd3d-shader/dxbc.c | 60 ++++++++++++++++++++++++++++------------ libs/vkd3d-shader/sm4.h | 25 +++++++++++++---- 2 files changed, 63 insertions(+), 22 deletions(-) diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c index fab19046..338499cd 100644 --- a/libs/vkd3d-shader/dxbc.c +++ b/libs/vkd3d-shader/dxbc.c @@ -1123,8 +1123,9 @@ static bool shader_sm4_read_param(struct vkd3d_sm4_data *priv, const DWORD **ptr } param->data_type = data_type; - if (token & VKD3D_SM4_REGISTER_MODIFIER) + if (token & VKD3D_SM4_EXTENDED_OPERAND) { + unsigned int type; DWORD m; if (*ptr >= end) @@ -1133,27 +1134,52 @@ static bool shader_sm4_read_param(struct vkd3d_sm4_data *priv, const DWORD **ptr return false; } m = *(*ptr)++; + type = m & VKD3D_SM4_EXTENDED_OPERAND_TYPE_MASK; - switch (m) + if (type == VKD3D_SM4_EXTENDED_OPERAND_MODIFIER) { - case VKD3D_SM4_REGISTER_MODIFIER_NEGATE: - *modifier = VKD3DSPSM_NEG; - break; + unsigned int min_precis = (m & VKD3D_SM4_REGISTER_MIN_PRECIS_MASK) >> VKD3D_SM4_REGISTER_MIN_PRECIS_SHIFT; + unsigned int op_mod = (m & VKD3D_SM4_REGISTER_MODIFIER_MASK) >> VKD3D_SM4_REGISTER_MODIFIER_SHIFT; + unsigned int non_uniform = + (m & VKD3D_SM4_REGISTER_NON_UNIFORM_MASK) >> VKD3D_SM4_REGISTER_NON_UNIFORM_SHIFT; - case VKD3D_SM4_REGISTER_MODIFIER_ABS: - *modifier = VKD3DSPSM_ABS; - break; + switch (op_mod & (VKD3D_SM4_REGISTER_MODIFIER_ABS | VKD3D_SM4_REGISTER_MODIFIER_NEGATE)) + { + case VKD3D_SM4_REGISTER_MODIFIER_NEGATE: + *modifier = VKD3DSPSM_NEG; + break; - case VKD3D_SM4_REGISTER_MODIFIER_ABS_NEGATE: - *modifier = VKD3DSPSM_ABSNEG; - break; + case VKD3D_SM4_REGISTER_MODIFIER_ABS: + *modifier = VKD3DSPSM_ABS; + break; - default: - FIXME("Skipping modifier 0x%08x.\n", m); - /* fall-through */ - case VKD3D_SM4_REGISTER_MODIFIER_NONE: - *modifier = VKD3DSPSM_NONE; - break; + case VKD3D_SM4_REGISTER_MODIFIER_ABS | VKD3D_SM4_REGISTER_MODIFIER_NEGATE: + *modifier = VKD3DSPSM_ABSNEG; + break; + + default: + *modifier = VKD3DSPSM_NONE; + break; + } + op_mod &= ~(VKD3D_SM4_REGISTER_MODIFIER_ABS | VKD3D_SM4_REGISTER_MODIFIER_NEGATE); + if (op_mod) + FIXME("Skipping modifier flags %#x.\n", op_mod); + + if (min_precis) + WARN("Ignoring minimum precision %#x.\n", min_precis); + + if (non_uniform) + FIXME("Ignoring extended modifier NON_UNIFORM.\n"); + } + else if (type) + { + FIXME("Unhandled extended operand type %#x.\n", type); + } + + if (m & VKD3D_SM4_EXTENDED_OPERAND) + { + FIXME("Skipping second-order extended operand.\n"); + *ptr += *ptr < end; } } else diff --git a/libs/vkd3d-shader/sm4.h b/libs/vkd3d-shader/sm4.h index 335f0161..495582ca 100644 --- a/libs/vkd3d-shader/sm4.h +++ b/libs/vkd3d-shader/sm4.h @@ -93,7 +93,18 @@ #define VKD3D_SM4_OPCODE_MASK 0xff -#define VKD3D_SM4_REGISTER_MODIFIER (0x1u << 31) +#define VKD3D_SM4_EXTENDED_OPERAND (0x1u << 31) + +#define VKD3D_SM4_EXTENDED_OPERAND_TYPE_MASK 0x1fu + +#define VKD3D_SM4_REGISTER_MODIFIER_SHIFT 6 +#define VKD3D_SM4_REGISTER_MODIFIER_MASK (0xffu << VKD3D_SM4_REGISTER_MODIFIER_SHIFT) + +#define VKD3D_SM4_REGISTER_MIN_PRECIS_SHIFT 14 +#define VKD3D_SM4_REGISTER_MIN_PRECIS_MASK (0x7u << VKD3D_SM4_REGISTER_MIN_PRECIS_SHIFT) + +#define VKD3D_SM4_REGISTER_NON_UNIFORM_SHIFT 17 +#define VKD3D_SM4_REGISTER_NON_UNIFORM_MASK (0x1u << VKD3D_SM4_REGISTER_NON_UNIFORM_SHIFT) #define VKD3D_SM4_ADDRESSING_SHIFT2 28 #define VKD3D_SM4_ADDRESSING_MASK2 (0x3u << VKD3D_SM4_ADDRESSING_SHIFT2) @@ -381,12 +392,16 @@ enum vkd3d_sm4_register_type VKD3D_SM5_RT_DEPTHOUT_LESS_EQUAL = 0x27, }; +enum vkd3d_sm4_extended_operand_type +{ + VKD3D_SM4_EXTENDED_OPERAND_NONE = 0x0, + VKD3D_SM4_EXTENDED_OPERAND_MODIFIER = 0x1, +}; + enum vkd3d_sm4_register_modifier { - VKD3D_SM4_REGISTER_MODIFIER_NONE = 0x01, - VKD3D_SM4_REGISTER_MODIFIER_NEGATE = 0x41, - VKD3D_SM4_REGISTER_MODIFIER_ABS = 0x81, - VKD3D_SM4_REGISTER_MODIFIER_ABS_NEGATE = 0xc1, + VKD3D_SM4_REGISTER_MODIFIER_NEGATE = 0x1, + VKD3D_SM4_REGISTER_MODIFIER_ABS = 0x2, }; enum vkd3d_sm4_output_primitive_type -- 2.32.0
Based on vkd3d-proton patches by Philip Rebohle and Hans-Kristian Arntzen. Signed-off-by: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/dxbc.c | 3 +- libs/vkd3d-shader/spirv.c | 42 ++++++++++++++++++++++-- libs/vkd3d-shader/vkd3d_shader_private.h | 7 ++++ tests/d3d12.c | 3 +- 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c index 338499cd..80662c2e 100644 --- a/libs/vkd3d-shader/dxbc.c +++ b/libs/vkd3d-shader/dxbc.c @@ -1121,6 +1121,7 @@ static bool shader_sm4_read_param(struct vkd3d_sm4_data *priv, const DWORD **ptr { param->type = register_type_table[register_type]; } + param->modifier = VKD3DSPRM_NONE; param->data_type = data_type; if (token & VKD3D_SM4_EXTENDED_OPERAND) @@ -1169,7 +1170,7 @@ static bool shader_sm4_read_param(struct vkd3d_sm4_data *priv, const DWORD **ptr WARN("Ignoring minimum precision %#x.\n", min_precis); if (non_uniform) - FIXME("Ignoring extended modifier NON_UNIFORM.\n"); + param->modifier = VKD3DSPRM_NONUNIFORM; } else if (type) { diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index 4e67af49..a0c60972 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -1842,7 +1842,8 @@ static bool vkd3d_spirv_compile_module(struct vkd3d_spirv_builder *builder, || vkd3d_spirv_capability_is_enabled(builder, SpvCapabilitySampledImageArrayDynamicIndexing) || vkd3d_spirv_capability_is_enabled(builder, SpvCapabilityStorageBufferArrayDynamicIndexing) || vkd3d_spirv_capability_is_enabled(builder, SpvCapabilityStorageTexelBufferArrayDynamicIndexingEXT) - || vkd3d_spirv_capability_is_enabled(builder, SpvCapabilityStorageImageArrayDynamicIndexing)) + || vkd3d_spirv_capability_is_enabled(builder, SpvCapabilityStorageImageArrayDynamicIndexing) + || vkd3d_spirv_capability_is_enabled(builder, SpvCapabilityShaderNonUniformEXT)) vkd3d_spirv_build_op_extension(&stream, "SPV_EXT_descriptor_indexing"); if (builder->ext_instr_set_glsl_450) @@ -2675,6 +2676,15 @@ static void vkd3d_dxbc_compiler_emit_descriptor_binding_for_reg(struct vkd3d_dxb vkd3d_dxbc_compiler_emit_descriptor_binding(compiler, variable_id, &binding); } +static void vkd3d_dxbc_compiler_decorate_nonuniform(struct vkd3d_dxbc_compiler *compiler, + uint32_t expression_id) +{ + struct vkd3d_spirv_builder *builder = &compiler->spirv_builder; + + vkd3d_spirv_enable_capability(builder, SpvCapabilityShaderNonUniformEXT); + vkd3d_spirv_build_op_decorate(builder, expression_id, SpvDecorationNonUniformEXT, NULL, 0); +} + static const struct vkd3d_symbol *vkd3d_dxbc_compiler_put_symbol(struct vkd3d_dxbc_compiler *compiler, const struct vkd3d_symbol *symbol) { @@ -3242,6 +3252,9 @@ static uint32_t vkd3d_dxbc_compiler_get_descriptor_index(struct vkd3d_dxbc_compi index.offset -= binding_base_idx; index_id = vkd3d_dxbc_compiler_emit_register_addressing(compiler, &index); + /* AMD drivers rely on the index being marked as nonuniform */ + if (reg->modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, index_id); return index_id; } @@ -3319,6 +3332,8 @@ static void vkd3d_dxbc_compiler_emit_dereference_register(struct vkd3d_dxbc_comp ptr_type_id = vkd3d_spirv_get_op_type_pointer(builder, register_info->storage_class, type_id); register_info->id = vkd3d_spirv_build_op_access_chain(builder, ptr_type_id, register_info->id, indexes, index_count); + if (reg->modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, register_info->id); } } @@ -7992,8 +8007,16 @@ static void vkd3d_dxbc_compiler_prepare_image(struct vkd3d_dxbc_compiler *compil return; } - image->image_id = load ? vkd3d_spirv_build_op_load(builder, - image->image_type_id, image->id, SpvMemoryAccessMaskNone) : 0; + if (load) + { + image->image_id = vkd3d_spirv_build_op_load(builder, image->image_type_id, image->id, SpvMemoryAccessMaskNone); + if (resource_reg->modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, image->image_id); + } + else + { + image->image_id = 0; + } image->image_type_id = vkd3d_dxbc_compiler_get_image_type_id(compiler, resource_reg, &symbol->info.resource.range, image->resource_type_info, @@ -8022,9 +8045,14 @@ static void vkd3d_dxbc_compiler_prepare_image(struct vkd3d_dxbc_compiler *compil sampler_id = vkd3d_spirv_build_op_load(builder, vkd3d_spirv_get_op_type_sampler(builder), sampler_var_id, SpvMemoryAccessMaskNone); + if (sampler_reg->modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, sampler_id); + sampled_image_type_id = vkd3d_spirv_get_op_type_sampled_image(builder, image->image_type_id); image->sampled_image_id = vkd3d_spirv_build_op_sampled_image(builder, sampled_image_type_id, image->image_id, sampler_id); + if (resource_reg->modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, image->sampled_image_id); } else { @@ -8375,6 +8403,8 @@ static void vkd3d_dxbc_compiler_emit_ld_raw_structured_srv_uav(struct vkd3d_dxbc indices[1] = coordinate_id; ptr_id = vkd3d_spirv_build_op_access_chain(builder, ptr_type_id, resource_symbol->id, indices, 2); + if (resource->reg.modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, ptr_id); constituents[j++] = vkd3d_spirv_build_op_load(builder, texel_type_id, ptr_id, SpvMemoryAccessMaskNone); } } @@ -8517,6 +8547,8 @@ static void vkd3d_dxbc_compiler_emit_store_uav_raw_structured(struct vkd3d_dxbc_ indices[1] = coordinate_id; ptr_id = vkd3d_spirv_build_op_access_chain(builder, ptr_type_id, resource_symbol->id, indices, 2); + if (dst->reg.modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, ptr_id); vkd3d_spirv_build_op_store(builder, ptr_id, data_id, SpvMemoryAccessMaskNone); } } @@ -8894,6 +8926,8 @@ static void vkd3d_dxbc_compiler_emit_atomic_instruction(struct vkd3d_dxbc_compil pointer_id = vkd3d_spirv_build_op_image_texel_pointer(builder, ptr_type_id, image.id, coordinate_id, sample_id); } + if (resource->reg.modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, pointer_id); } val_id = vkd3d_dxbc_compiler_emit_load_src_with_type(compiler, &src[1], VKD3DSP_WRITEMASK_0, component_type); @@ -8942,6 +8976,8 @@ static void vkd3d_dxbc_compiler_emit_bufinfo(struct vkd3d_dxbc_compiler *compile type_id = vkd3d_spirv_get_type_id(builder, VKD3D_SHADER_COMPONENT_UINT, 1); val_id = vkd3d_spirv_build_op_image_query_size(builder, type_id, image.image_id); + if (src->reg.modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, image.id); write_mask = VKD3DSP_WRITEMASK_0; } diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 9948045e..a70cab59 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -455,6 +455,12 @@ enum vkd3d_immconst_type VKD3D_IMMCONST_VEC4, }; +enum vkd3d_shader_register_modifier +{ + VKD3DSPRM_NONE = 0, + VKD3DSPRM_NONUNIFORM = 1, +}; + enum vkd3d_shader_src_modifier { VKD3DSPSM_NONE = 0, @@ -612,6 +618,7 @@ struct vkd3d_shader_register_index struct vkd3d_shader_register { enum vkd3d_shader_register_type type; + enum vkd3d_shader_register_modifier modifier; enum vkd3d_data_type data_type; struct vkd3d_shader_register_index idx[3]; enum vkd3d_immconst_type immconst_type; diff --git a/tests/d3d12.c b/tests/d3d12.c index bd3ef624..ecda1272 100644 --- a/tests/d3d12.c +++ b/tests/d3d12.c @@ -34960,7 +34960,7 @@ static void test_unbounded_resource_arrays(void) D3D12_RESOURCE_STATE_UNORDERED_ACCESS, D3D12_RESOURCE_STATE_COPY_SOURCE); get_buffer_readback_with_command_list(output_buffers[i], DXGI_FORMAT_R32_UINT, &rb, queue, command_list); /* Buffers at index >= 64 are aliased. */ - todo_if(i != 10 && i != 74) + todo_if(i != 74) check_readback_data_uint(&rb, NULL, (i < 64 ? 63 - i : 127 - i) ^ 0x35, 0); release_resource_readback(&rb); reset_command_list(command_list, context.allocator); @@ -35107,7 +35107,6 @@ static void test_unbounded_samplers(void) { unsigned int value = get_readback_uint(&rb, i, 0, 0); unsigned int expected = (i & 1) ? 100 : 10; - todo_if(i & 1) ok(value == expected, "Got %u, expected %u at %u.\n", value, expected, i); } release_resource_readback(&rb); -- 2.32.0
On Mon, 16 Aug 2021 at 06:16, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
@@ -1169,7 +1170,7 @@ static bool shader_sm4_read_param(struct vkd3d_sm4_data *priv, const DWORD **ptr WARN("Ignoring minimum precision %#x.\n", min_precis);
if (non_uniform) - FIXME("Ignoring extended modifier NON_UNIFORM.\n"); + param->modifier = VKD3DSPRM_NONUNIFORM; }
Should there be trace.c changes to go along with this?
@@ -3242,6 +3252,9 @@ static uint32_t vkd3d_dxbc_compiler_get_descriptor_index(struct vkd3d_dxbc_compi
index.offset -= binding_base_idx; index_id = vkd3d_dxbc_compiler_emit_register_addressing(compiler, &index); + /* AMD drivers rely on the index being marked as nonuniform */ + if (reg->modifier == VKD3DSPRM_NONUNIFORM) + vkd3d_dxbc_compiler_decorate_nonuniform(compiler, index_id);
Is that radv, amdvlk, or radeonsi? Any particular version?
+enum vkd3d_shader_register_modifier +{ + VKD3DSPRM_NONE = 0, + VKD3DSPRM_NONUNIFORM = 1, +}; + enum vkd3d_shader_src_modifier { VKD3DSPSM_NONE = 0, @@ -612,6 +618,7 @@ struct vkd3d_shader_register_index struct vkd3d_shader_register { enum vkd3d_shader_register_type type; + enum vkd3d_shader_register_modifier modifier; enum vkd3d_data_type data_type; struct vkd3d_shader_register_index idx[3]; enum vkd3d_immconst_type immconst_type;
It seems tempting to simply add a "bool non_uniform;" field.
On Mon, 16 Aug 2021 at 06:16, Conor McCarthy <cmccarthy(a)codeweavers.com> wrote:
@@ -1123,8 +1123,9 @@ static bool shader_sm4_read_param(struct vkd3d_sm4_data *priv, const DWORD **ptr } param->data_type = data_type;
- if (token & VKD3D_SM4_REGISTER_MODIFIER) + if (token & VKD3D_SM4_EXTENDED_OPERAND) { + unsigned int type; DWORD m; "enum vkd3d_sm4_extended_operand_type type;", technically.
@@ -1133,27 +1134,52 @@ static bool shader_sm4_read_param(struct vkd3d_sm4_data *priv, const DWORD **ptr return false; } m = *(*ptr)++; + type = m & VKD3D_SM4_EXTENDED_OPERAND_TYPE_MASK;
- switch (m) + if (type == VKD3D_SM4_EXTENDED_OPERAND_MODIFIER) { - case VKD3D_SM4_REGISTER_MODIFIER_NEGATE: - *modifier = VKD3DSPSM_NEG; - break; + unsigned int min_precis = (m & VKD3D_SM4_REGISTER_MIN_PRECIS_MASK) >> VKD3D_SM4_REGISTER_MIN_PRECIS_SHIFT; + unsigned int op_mod = (m & VKD3D_SM4_REGISTER_MODIFIER_MASK) >> VKD3D_SM4_REGISTER_MODIFIER_SHIFT; + unsigned int non_uniform = + (m & VKD3D_SM4_REGISTER_NON_UNIFORM_MASK) >> VKD3D_SM4_REGISTER_NON_UNIFORM_SHIFT;
Extracting the "non-uniform" and "minimum precision" modifiers are separate changes, and should be in their own patches.
+ switch (op_mod & (VKD3D_SM4_REGISTER_MODIFIER_ABS | VKD3D_SM4_REGISTER_MODIFIER_NEGATE)) + { Is there still a reason to handle these as flags? It seems somewhat of an arbitrary change now.
+ if (min_precis) + WARN("Ignoring minimum precision %#x.\n", min_precis); + + if (non_uniform) + FIXME("Ignoring extended modifier NON_UNIFORM.\n"); + } + else if (type) + { + FIXME("Unhandled extended operand type %#x.\n", type); + } + We leave "*modifier" uninitialised here. We should probably also check for any unhandled but set bits, and generate a message.
participants (2)
-
Conor McCarthy -
Henri Verbeet