[PATCH v2 0/3] MR629: vkd3d-shader/dxil: Implement DX intrinsic GetDimensions.
-- v2: vkd3d-shader/dxil: Implement DX intrinsic GetDimensions. vkd3d-shader/dxil: Move the resource kind helper functions up. vkd3d-shader/spirv: Introduce a sample info flag value for a scalar uint dst parameter. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629
From: Conor McCarthy <cmccarthy(a)codeweavers.com> The result of sample info in Shader Model 6 is always uint, and must be a scalar for insertion into a vector temp. --- libs/vkd3d-shader/d3d_asm.c | 1 + libs/vkd3d-shader/spirv.c | 3 +++ libs/vkd3d-shader/vkd3d_shader_private.h | 1 + 3 files changed, 5 insertions(+) diff --git a/libs/vkd3d-shader/d3d_asm.c b/libs/vkd3d-shader/d3d_asm.c index 3f86bd459..bec928686 100644 --- a/libs/vkd3d-shader/d3d_asm.c +++ b/libs/vkd3d-shader/d3d_asm.c @@ -1689,6 +1689,7 @@ static void shader_dump_instruction_flags(struct vkd3d_d3d_asm_compiler *compile { case VKD3DSI_NONE: break; case VKD3DSI_SAMPLE_INFO_UINT: shader_addline(buffer, "_uint"); break; + case VKD3DSI_SAMPLE_INFO_UINT_DST: shader_addline(buffer, "_uintDst"); break; default: shader_addline(buffer, "_unrecognized(%#x)", ins->flags); } break; diff --git a/libs/vkd3d-shader/spirv.c b/libs/vkd3d-shader/spirv.c index a3baeea75..eab8d59be 100644 --- a/libs/vkd3d-shader/spirv.c +++ b/libs/vkd3d-shader/spirv.c @@ -9199,6 +9199,9 @@ static void spirv_compiler_emit_sample_info(struct spirv_compiler *compiler, val_id = spirv_compiler_emit_query_sample_count(compiler, src); + if (instruction->flags == VKD3DSI_SAMPLE_INFO_UINT_DST) + return spirv_compiler_emit_store_dst(compiler, dst, val_id); + constituents[0] = val_id; for (i = 1; i < VKD3D_VEC4_SIZE; ++i) constituents[i] = spirv_compiler_get_constant_uint(compiler, 0); diff --git a/libs/vkd3d-shader/vkd3d_shader_private.h b/libs/vkd3d-shader/vkd3d_shader_private.h index 1030adf98..7f22f2442 100644 --- a/libs/vkd3d-shader/vkd3d_shader_private.h +++ b/libs/vkd3d-shader/vkd3d_shader_private.h @@ -762,6 +762,7 @@ enum vkd3d_tessellator_domain #define VKD3DSI_RESINFO_RCP_FLOAT 0x1 #define VKD3DSI_RESINFO_UINT 0x2 #define VKD3DSI_SAMPLE_INFO_UINT 0x1 +#define VKD3DSI_SAMPLE_INFO_UINT_DST 0x2 #define VKD3DSI_SAMPLER_COMPARISON_MODE 0x1 #define VKD3DSI_SHIFT_UNMASKED 0x1 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/dxil.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index a001f6f06..f45a795d7 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -1904,6 +1904,16 @@ static const struct sm6_type *sm6_parser_get_type(struct sm6_parser *sm6, uint64 return &sm6->types[type_id]; } +static bool resource_kind_is_texture(enum dxil_resource_kind kind) +{ + return kind >= RESOURCE_KIND_TEXTURE1D && kind <= RESOURCE_KIND_TEXTURECUBEARRAY; +} + +static bool resource_kind_is_multisampled(enum dxil_resource_kind kind) +{ + return kind == RESOURCE_KIND_TEXTURE2DMS || kind == RESOURCE_KIND_TEXTURE2DMSARRAY; +} + static int global_symbol_compare(const void *a, const void *b) { return vkd3d_u32_compare(((const struct sm6_symbol *)a)->id, ((const struct sm6_symbol *)b)->id); @@ -6380,16 +6390,6 @@ static bool sm6_parser_resources_load_register_range(struct sm6_parser *sm6, return true; } -static bool resource_kind_is_texture(enum dxil_resource_kind kind) -{ - return kind >= RESOURCE_KIND_TEXTURE1D && kind <= RESOURCE_KIND_TEXTURECUBEARRAY; -} - -static bool resource_kind_is_multisampled(enum dxil_resource_kind kind) -{ - return kind == RESOURCE_KIND_TEXTURE2DMS || kind == RESOURCE_KIND_TEXTURE2DMSARRAY; -} - static enum vkd3d_shader_resource_type shader_resource_type_from_dxil_resource_kind(enum dxil_resource_kind kind) { if (resource_kind_is_texture(kind)) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- libs/vkd3d-shader/dxil.c | 79 ++++++++++++++++++++++++++++ tests/hlsl/getdimensions.shader_test | 4 +- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/dxil.c b/libs/vkd3d-shader/dxil.c index f45a795d7..bd1ed5fa5 100644 --- a/libs/vkd3d-shader/dxil.c +++ b/libs/vkd3d-shader/dxil.c @@ -372,6 +372,7 @@ enum dx_intrinsic_opcode DX_TEXTURE_LOAD = 66, DX_TEXTURE_STORE = 67, DX_BUFFER_LOAD = 68, + DX_GET_DIMENSIONS = 72, DX_DERIV_COARSEX = 83, DX_DERIV_COARSEY = 84, DX_DERIV_FINEX = 85, @@ -2323,6 +2324,23 @@ static void instruction_dst_param_init_ssa_vector(struct vkd3d_shader_instructio dst->u.reg = param->reg; } +static bool instruction_dst_param_init_temp_vector(struct vkd3d_shader_instruction *ins, struct sm6_parser *sm6) +{ + struct sm6_value *dst = sm6_parser_get_current_value(sm6); + struct vkd3d_shader_dst_param *param; + + if (!(param = instruction_dst_params_alloc(ins, 1, sm6))) + return false; + + vsir_dst_param_init(param, VKD3DSPR_TEMP, vkd3d_data_type_from_sm6_type(sm6_type_get_scalar_type(dst->type, 0)), 1); + param->write_mask = VKD3DSP_WRITEMASK_ALL; + param->reg.idx[0].offset = 0; + param->reg.dimension = VSIR_DIMENSION_VEC4; + dst->u.reg = param->reg; + + return true; +} + /* Recurse through the block tree while maintaining a current value count. The current * count is the sum of the global count plus all declarations within the current function. * Store into value_capacity the highest count seen. */ @@ -3892,6 +3910,63 @@ static void sm6_parser_emit_dx_create_handle(struct sm6_parser *sm6, enum dx_int ins->handler_idx = VKD3DSIH_NOP; } +static void sm6_parser_emit_dx_get_dimensions(struct sm6_parser *sm6, enum dx_intrinsic_opcode op, + const struct sm6_value **operands, struct function_emission_state *state) +{ + struct vkd3d_shader_instruction *ins = state->ins; + struct vkd3d_shader_src_param *src_params; + unsigned int is_texture, component_count; + enum dxil_resource_kind resource_kind; + const struct sm6_value *resource; + + resource = operands[0]; + if (!sm6_value_validate_is_handle(resource, sm6)) + return; + is_texture = resource->u.handle.d->resource_type != VKD3D_SHADER_RESOURCE_BUFFER; + resource_kind = resource->u.handle.d->kind; + + instruction_init_with_resource(ins, is_texture ? VKD3DSIH_RESINFO : VKD3DSIH_BUFINFO, resource, sm6); + + if (!(src_params = instruction_src_params_alloc(ins, 1 + is_texture, sm6))) + return; + src_param_init_vector_from_reg(&src_params[is_texture], &resource->u.handle.reg); + + if (is_texture) + { + /* DXIL does not have an instrinsic for sample info, and resinfo is expected to return + * the sample count in .w for MS textures. The result is always a struct of 4 x uint32. */ + ins->flags = VKD3DSI_RESINFO_UINT; + src_param_init_from_value(&src_params[0], operands[1]); + component_count = VKD3D_VEC4_SIZE; + + instruction_dst_param_init_temp_vector(ins++, sm6); + state->temp_idx = 1; + + if (resource_kind_is_multisampled(resource_kind)) + { + vsir_instruction_init(ins, &sm6->p.location, VKD3DSIH_SAMPLE_INFO); + ins->flags = VKD3DSI_SAMPLE_INFO_UINT_DST; + + if (!(src_params = instruction_src_params_alloc(ins, 1, sm6))) + return; + src_param_init_vector_from_reg(&src_params[0], &resource->u.handle.reg); + src_params[0].swizzle = 0; + + if (!instruction_dst_param_init_temp_vector(ins, sm6)) + return; + ins->dst->write_mask = VKD3DSP_WRITEMASK_3; + + state->ins = ins; + ++state->code_block->instruction_count; + } + } + else + { + component_count = 1 + (resource_kind == RESOURCE_KIND_STRUCTUREDBUFFER); + instruction_dst_param_init_ssa_vector(ins, component_count, sm6); + } +} + static enum vkd3d_shader_opcode sm6_dx_map_tertiary_op(enum dx_intrinsic_opcode op) { switch (op) @@ -4254,6 +4329,7 @@ struct sm6_dx_opcode_info e -> half/float g -> half/float/double H -> handle + D -> Dimensions S -> splitdouble v -> void o -> overloaded @@ -4281,6 +4357,7 @@ static const struct sm6_dx_opcode_info sm6_dx_op_table[] = [DX_FMAX ] = {"g", "RR", sm6_parser_emit_dx_binary}, [DX_FMIN ] = {"g", "RR", sm6_parser_emit_dx_binary}, [DX_FRC ] = {"g", "R", sm6_parser_emit_dx_unary}, + [DX_GET_DIMENSIONS ] = {"D", "Hi", sm6_parser_emit_dx_get_dimensions}, [DX_IBFE ] = {"m", "iiR", sm6_parser_emit_dx_tertiary}, [DX_HCOS ] = {"g", "R", sm6_parser_emit_dx_unary}, [DX_HSIN ] = {"g", "R", sm6_parser_emit_dx_unary}, @@ -4351,6 +4428,8 @@ static bool sm6_parser_validate_operand_type(struct sm6_parser *sm6, const struc return sm6_type_is_floating_point(type); case 'H': return (is_return || sm6_value_is_handle(value)) && type == sm6->handle_type; + case 'D': + return sm6_type_is_struct(type) && !strcmp(type->u.struc->name, "dx.types.Dimensions"); case 'S': return sm6_type_is_struct(type) && !strcmp(type->u.struc->name, "dx.types.splitdouble"); case 'v': diff --git a/tests/hlsl/getdimensions.shader_test b/tests/hlsl/getdimensions.shader_test index 01d12f87a..4d781b320 100644 --- a/tests/hlsl/getdimensions.shader_test +++ b/tests/hlsl/getdimensions.shader_test @@ -28,7 +28,7 @@ float4 main() : sv_target } [test] -todo(sm>=6) draw quad +draw quad probe all rgba (2.0, 3.0, 2.0, 3.0) [texture 1] @@ -53,5 +53,5 @@ float4 main() : sv_target } [test] -todo(sm>=6) draw quad +draw quad probe all rgba (2.0, 2.0, 1.0, 2.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
+ return; + is_texture = resource->u.handle.d->resource_type != VKD3D_SHADER_RESOURCE_BUFFER; + resource_kind = resource->u.handle.d->kind; + + instruction_init_with_resource(ins, is_texture ? VKD3DSIH_RESINFO : VKD3DSIH_BUFINFO, resource, sm6); + + if (!(src_params = instruction_src_params_alloc(ins, 1 + is_texture, sm6))) + return; + src_param_init_vector_from_reg(&src_params[is_texture], &resource->u.handle.reg); + + if (is_texture) + { + /* DXIL does not have an instrinsic for sample info, and resinfo is expected to return + * the sample count in .w for MS textures. The result is always a struct of 4 x uint32. */ + ins->flags = VKD3DSI_RESINFO_UINT; + src_param_init_from_value(&src_params[0], operands[1]); If the resource is a buffer `operands[1]` is expected to be undefined? Should this be validated, consistently with what you do elsewhere?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629#note_61401
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
+ ins->flags = VKD3DSI_RESINFO_UINT; + src_param_init_from_value(&src_params[0], operands[1]); + component_count = VKD3D_VEC4_SIZE; + + instruction_dst_param_init_temp_vector(ins++, sm6); + state->temp_idx = 1; + + if (resource_kind_is_multisampled(resource_kind)) + { + vsir_instruction_init(ins, &sm6->p.location, VKD3DSIH_SAMPLE_INFO); + ins->flags = VKD3DSI_SAMPLE_INFO_UINT_DST; + + if (!(src_params = instruction_src_params_alloc(ins, 1, sm6))) + return; + src_param_init_vector_from_reg(&src_params[0], &resource->u.handle.reg); + src_params[0].swizzle = 0; Maybe `VKD3D_SHADER_SWIZZLE(X, X, X, X)`?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629#note_61402
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
dst->u.reg = param->reg; }
+static bool instruction_dst_param_init_temp_vector(struct vkd3d_shader_instruction *ins, struct sm6_parser *sm6) +{ + struct sm6_value *dst = sm6_parser_get_current_value(sm6); + struct vkd3d_shader_dst_param *param; + + if (!(param = instruction_dst_params_alloc(ins, 1, sm6))) + return false; + + vsir_dst_param_init(param, VKD3DSPR_TEMP, vkd3d_data_type_from_sm6_type(sm6_type_get_scalar_type(dst->type, 0)), 1); + param->write_mask = VKD3DSP_WRITEMASK_ALL; + param->reg.idx[0].offset = 0; + param->reg.dimension = VSIR_DIMENSION_VEC4; + dst->u.reg = param->reg;
Hmm, is this correct? If two instructions use this temporary register (which is always hardcoded at `r0`, isn't it?) and the former value is then recalled, it will be replaced with `r0`, but the register will have been, by then, overwritten with the result of the second instruction. Maybe the content of `r0` should immediately be copied to a SSA value? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629#note_61403
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/vkd3d_shader_private.h:
#define VKD3DSI_RESINFO_RCP_FLOAT 0x1 #define VKD3DSI_RESINFO_UINT 0x2 #define VKD3DSI_SAMPLE_INFO_UINT 0x1 +#define VKD3DSI_SAMPLE_INFO_UINT_DST 0x2 Maybe `UINT_SCALAR` is more descriptive than `UINT_DST`? It seems that the point here is having a scalar value rather than any property related to its destination.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629#note_61404
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/dxil.c:
+ instruction_init_with_resource(ins, is_texture ? VKD3DSIH_RESINFO : VKD3DSIH_BUFINFO, resource, sm6); + + if (!(src_params = instruction_src_params_alloc(ins, 1 + is_texture, sm6))) + return; + src_param_init_vector_from_reg(&src_params[is_texture], &resource->u.handle.reg); + + if (is_texture) + { + /* DXIL does not have an instrinsic for sample info, and resinfo is expected to return + * the sample count in .w for MS textures. The result is always a struct of 4 x uint32. */ + ins->flags = VKD3DSI_RESINFO_UINT; + src_param_init_from_value(&src_params[0], operands[1]); + component_count = VKD3D_VEC4_SIZE; + + instruction_dst_param_init_temp_vector(ins++, sm6); + state->temp_idx = 1; This feels fragile: if in the future somewhere else `r1` is used and `temp_idx = 2` is put somewhere else, then the reference `r1` becomes illegal. Maybe setting `temp_idx` should filter with a `max()` and be moved inside `instruction_dst_param_init_temp_vector()`?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629#note_61405
I like it more now. There are still a few points, though. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629#note_61406
On Thu Feb 15 22:52:57 2024 +0000, Giovanni Mascellani wrote:
Hmm, is this correct? If two instructions use this temporary register (which is always hardcoded at `r0`, isn't it?) and the former value is then recalled, it will be replaced with `r0`, but the register will have been, by then, overwritten with the result of the second instruction. Maybe the content of `r0` should immediately be copied to a SSA value? It relies on dxcompiler always extracting the scalars immediately after the vector result is emitted. It may be simpler to validate that than emit a `MOV`.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629#note_61431
On Thu Feb 15 22:52:58 2024 +0000, Giovanni Mascellani wrote:
This feels fragile: if in the future somewhere else `r1` is used and `temp_idx = 2` is put somewhere else, then the reference `r1` becomes illegal. Maybe setting `temp_idx` should filter with a `max()` and be moved inside `instruction_dst_param_init_temp_vector()`? So long as values are extracted immediately, and temps elsewhere are only used to construct src params, there can be no temp register conflicts.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/629#note_61432
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Giovanni Mascellani (＠giomasce)