[PATCH 0/2] MR343: Draft: vkd3d-shader/hlsl: Make deref->data_type available for the whole deref's lifetime.
I marked this a draft because it may be a somewhat orthogonal step, but IRRC @giomasce suggested it. While a deref's data type can be computed from the variable's data type as long as the deref is still represented as a path (as we did with hlsl_deref_get_type()), we need the deref.data_type field for when the deref is represented as an offset. While we could get rid of this deref.data_type in the future, if we manage to transform the HLSL IR instructions directly to VSIR withot requiring lowering the derefs to their offset representation, this would take a while. So, this patch makes the deref.data_type field available during the whole lifetime of the deref. Which makes deref.data_type easier to understand (since it can be used anytime now) and we no longer have to call hlsl_deref_get_type(). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343
From: Francisco Casas <fcasas(a)codeweavers.com> While a deref's data type can be computed from the variable's data type as long as the deref is still represented as a path (as we did with hlsl_deref_get_type()), we need the deref.data_type field for when the deref is represented as an offset. While we could get rid of it in the future, if we manage to transform the HLSL IR instructions directly to VSIR withot requiring lowering the derefs to their offset representation, this would take a while. So, this patch makes the deref.data_type field available during the whole lifetime of the deref. Which makes deref.data_type easier to understand (since it can be used anytime now) and we no longer have to call hlsl_deref_get_type(). --- libs/vkd3d-shader/hlsl.c | 39 ++++++++++++++++++++++---------- libs/vkd3d-shader/hlsl.h | 7 +++--- libs/vkd3d-shader/hlsl_codegen.c | 4 ++-- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 09580970..52766671 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -503,6 +503,8 @@ static bool init_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hl deref->path_len = path_len; deref->offset.node = NULL; + deref->data_type = NULL; + if (path_len == 0) { deref->path = NULL; @@ -519,6 +521,17 @@ static bool init_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hl return true; } +void hlsl_deref_compute_data_type(struct hlsl_ctx *ctx, struct hlsl_deref *deref) +{ + struct hlsl_type *type = deref->var->data_type; + unsigned int i; + + for (i = 0; i < deref->path_len; ++i) + type = hlsl_get_element_type_from_path_index(ctx, type, deref->path[i].node); + + deref->data_type = type; +} + bool hlsl_init_deref_from_index_chain(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_node *chain) { struct hlsl_ir_index *index; @@ -581,23 +594,14 @@ bool hlsl_init_deref_from_index_chain(struct hlsl_ctx *ctx, struct hlsl_deref *d } assert(deref->path_len == load->src.path_len + chain_len); + hlsl_deref_compute_data_type(ctx, deref); + return true; } struct hlsl_type *hlsl_deref_get_type(struct hlsl_ctx *ctx, const struct hlsl_deref *deref) { - struct hlsl_type *type; - unsigned int i; - - assert(deref); - - if (deref->offset.node) - return deref->data_type; - - type = deref->var->data_type; - for (i = 0; i < deref->path_len; ++i) - type = hlsl_get_element_type_from_path_index(ctx, type, deref->path[i].node); - return type; + return deref->data_type; } /* Initializes a deref from another deref (prefix) and a component index. @@ -646,6 +650,8 @@ static bool init_deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_bl assert(deref_path_len == deref->path_len); + deref->data_type = path_type; + return true; } @@ -1110,6 +1116,8 @@ bool hlsl_copy_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, const struc for (i = 0; i < deref->path_len; ++i) hlsl_src_from_node(&deref->path[i], other->path[i].node); + deref->data_type = other->data_type; + return true; } @@ -1132,6 +1140,7 @@ void hlsl_init_simple_deref_from_var(struct hlsl_deref *deref, struct hlsl_ir_va { memset(deref, 0, sizeof(*deref)); deref->var = var; + deref->data_type = var->data_type; } static void init_node(struct hlsl_ir_node *node, enum hlsl_ir_node_type type, @@ -1174,6 +1183,7 @@ struct hlsl_ir_node *hlsl_new_store_index(struct hlsl_ctx *ctx, const struct hls hlsl_src_from_node(&store->lhs.path[i], lhs->path[i].node); if (idx) hlsl_src_from_node(&store->lhs.path[lhs->path_len], idx); + hlsl_deref_compute_data_type(ctx, &store->lhs); hlsl_src_from_node(&store->rhs, rhs); @@ -1351,6 +1361,7 @@ struct hlsl_ir_load *hlsl_new_load_index(struct hlsl_ctx *ctx, const struct hlsl hlsl_src_from_node(&load->src.path[i], deref->path[i].node); if (idx) hlsl_src_from_node(&load->src.path[deref->path_len], idx); + hlsl_deref_compute_data_type(ctx, &load->src); return load; } @@ -1365,6 +1376,8 @@ struct hlsl_ir_load *hlsl_new_load_parent(struct hlsl_ctx *ctx, const struct hls tmp_deref = *deref; tmp_deref.path_len = deref->path_len - 1; + hlsl_deref_compute_data_type(ctx, &tmp_deref); + return hlsl_new_load_index(ctx, &tmp_deref, NULL, loc); } @@ -1613,6 +1626,8 @@ static bool clone_deref(struct hlsl_ctx *ctx, struct clone_instr_map *map, for (i = 0; i < src->path_len; ++i) hlsl_src_from_node(&dst->path[i], map_instr(map, src->path[i].node)); + dst->data_type = src->data_type; + return true; } diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 04e681a9..5bc59add 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -615,10 +615,10 @@ struct hlsl_deref * components, within the pertaining regset), from the start of the variable, of the part * referenced. * The path is lowered to this single offset -- whose value may vary between SM1 and SM4 -- - * before writing the bytecode. - * Since the type information cannot longer be retrieved from the offset alone, the type is - * stored in the data_type field. */ + * before writing the bytecode. */ struct hlsl_src offset; + + /* Type of the part of the variable referenced by this deref. */ struct hlsl_type *data_type; }; @@ -1247,6 +1247,7 @@ unsigned int hlsl_map_swizzle(unsigned int swizzle, unsigned int writemask); unsigned int hlsl_swizzle_from_writemask(unsigned int writemask); struct hlsl_type *hlsl_deref_get_type(struct hlsl_ctx *ctx, const struct hlsl_deref *deref); +void hlsl_deref_compute_data_type(struct hlsl_ctx *ctx, struct hlsl_deref *deref); bool hlsl_component_index_range_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref, unsigned int *start, unsigned int *count); bool hlsl_regset_index_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref, diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 132f44a2..023f2e8d 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -146,8 +146,6 @@ static bool replace_deref_path_with_offset(struct hlsl_ctx *ctx, struct hlsl_der return true; } - deref->data_type = type; - if (!(offset = new_offset_instr_from_deref(ctx, &block, deref, &instr->loc))) return false; list_move_before(&instr->entry, &block.instrs); @@ -2181,6 +2179,8 @@ static bool lower_combined_samples(struct hlsl_ctx *ctx, struct hlsl_ir_node *in hlsl_copy_deref(ctx, &load->sampler, &load->resource); load->resource.var = var; + hlsl_deref_compute_data_type(ctx, &load->resource); + assert(hlsl_deref_get_type(ctx, &load->resource)->base_type == HLSL_TYPE_TEXTURE); assert(hlsl_deref_get_type(ctx, &load->sampler)->base_type == HLSL_TYPE_SAMPLER); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.c | 15 +++++---------- libs/vkd3d-shader/hlsl.h | 1 - libs/vkd3d-shader/hlsl.y | 2 +- libs/vkd3d-shader/hlsl_codegen.c | 12 +++++------- libs/vkd3d-shader/tpf.c | 6 +++--- 5 files changed, 14 insertions(+), 22 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 52766671..b5bc2980 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -599,11 +599,6 @@ bool hlsl_init_deref_from_index_chain(struct hlsl_ctx *ctx, struct hlsl_deref *d return true; } -struct hlsl_type *hlsl_deref_get_type(struct hlsl_ctx *ctx, const struct hlsl_deref *deref) -{ - return deref->data_type; -} - /* Initializes a deref from another deref (prefix) and a component index. * *block is initialized to contain the new constant node instructions used by the deref's path. */ static bool init_deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_block *block, @@ -617,7 +612,7 @@ static bool init_deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_bl hlsl_block_init(block); path_len = 0; - path_type = hlsl_deref_get_type(ctx, prefix); + path_type = prefix->data_type; path_index = index; while (!type_is_single_component(path_type)) { @@ -632,7 +627,7 @@ static bool init_deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_bl for (i = 0; i < prefix->path_len; ++i) hlsl_src_from_node(&deref->path[deref_path_len++], prefix->path[i].node); - path_type = hlsl_deref_get_type(ctx, prefix); + path_type = prefix->data_type; path_index = index; while (!type_is_single_component(path_type)) { @@ -1344,7 +1339,7 @@ struct hlsl_ir_load *hlsl_new_load_index(struct hlsl_ctx *ctx, const struct hlsl assert(!deref->offset.node); - type = hlsl_deref_get_type(ctx, deref); + type = deref->data_type; if (idx) type = hlsl_get_element_type_from_path_index(ctx, type, idx); @@ -1402,7 +1397,7 @@ struct hlsl_ir_node *hlsl_new_load_component(struct hlsl_ctx *ctx, struct hlsl_b if (!(load = hlsl_alloc(ctx, sizeof(*load)))) return NULL; - type = hlsl_deref_get_type(ctx, deref); + type = deref->data_type; comp_type = hlsl_type_get_component_type(ctx, type, comp); init_node(&load->node, HLSL_IR_LOAD, comp_type, loc); @@ -1453,7 +1448,7 @@ struct hlsl_ir_node *hlsl_new_resource_load(struct hlsl_ctx *ctx, hlsl_src_from_node(&load->cmp, params->cmp); load->sampling_dim = params->sampling_dim; if (load->sampling_dim == HLSL_SAMPLER_DIM_GENERIC) - load->sampling_dim = hlsl_deref_get_type(ctx, &load->resource)->sampler_dim; + load->sampling_dim = load->resource.data_type->sampler_dim; return &load->node; } diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 5bc59add..eeda2b25 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -1246,7 +1246,6 @@ unsigned int hlsl_combine_writemasks(unsigned int first, unsigned int second); unsigned int hlsl_map_swizzle(unsigned int swizzle, unsigned int writemask); unsigned int hlsl_swizzle_from_writemask(unsigned int writemask); -struct hlsl_type *hlsl_deref_get_type(struct hlsl_ctx *ctx, const struct hlsl_deref *deref); void hlsl_deref_compute_data_type(struct hlsl_ctx *ctx, struct hlsl_deref *deref); bool hlsl_component_index_range_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref, unsigned int *start, unsigned int *count); diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 67fd9f6f..30e3632a 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1766,7 +1766,7 @@ static struct hlsl_ir_node *add_assignment(struct hlsl_ctx *ctx, struct hlsl_blo if (!hlsl_init_deref_from_index_chain(ctx, &resource_deref, hlsl_ir_index(lhs)->val.node)) return NULL; - resource_type = hlsl_deref_get_type(ctx, &resource_deref); + resource_type = resource_deref.data_type; assert(resource_type->class == HLSL_CLASS_OBJECT); assert(resource_type->base_type == HLSL_TYPE_TEXTURE || resource_type->base_type == HLSL_TYPE_UAV); diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 023f2e8d..44b24f58 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -127,7 +127,7 @@ static struct hlsl_ir_node *new_offset_instr_from_deref(struct hlsl_ctx *ctx, st static bool replace_deref_path_with_offset(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_node *instr) { - struct hlsl_type *type; + struct hlsl_type *type = deref->data_type; struct hlsl_ir_node *offset; struct hlsl_block block; @@ -136,8 +136,6 @@ static bool replace_deref_path_with_offset(struct hlsl_ctx *ctx, struct hlsl_der /* register offsets shouldn't be used before this point is reached. */ assert(!deref->offset.node); - type = hlsl_deref_get_type(ctx, deref); - /* Instructions that directly refer to structs or arrays (instead of single-register components) * are removed later by dce. So it is not a problem to just cleanup their derefs. */ if (type->class == HLSL_CLASS_STRUCT || type->class == HLSL_CLASS_ARRAY) @@ -2181,8 +2179,8 @@ static bool lower_combined_samples(struct hlsl_ctx *ctx, struct hlsl_ir_node *in load->resource.var = var; hlsl_deref_compute_data_type(ctx, &load->resource); - assert(hlsl_deref_get_type(ctx, &load->resource)->base_type == HLSL_TYPE_TEXTURE); - assert(hlsl_deref_get_type(ctx, &load->sampler)->base_type == HLSL_TYPE_SAMPLER); + assert(load->resource.data_type->base_type == HLSL_TYPE_TEXTURE); + assert(load->sampler.data_type->base_type == HLSL_TYPE_SAMPLER); return true; } @@ -3307,7 +3305,7 @@ static bool track_object_components_sampler_dim(struct hlsl_ctx *ctx, struct hls load = hlsl_ir_resource_load(instr); var = load->resource.var; - regset = hlsl_type_get_regset(hlsl_deref_get_type(ctx, &load->resource)); + regset = hlsl_type_get_regset(load->resource.data_type); if (!hlsl_regset_index_from_deref(ctx, &load->resource, regset, &index)) return false; @@ -3352,7 +3350,7 @@ static bool track_object_components_usage(struct hlsl_ctx *ctx, struct hlsl_ir_n load = hlsl_ir_resource_load(instr); var = load->resource.var; - regset = hlsl_type_get_regset(hlsl_deref_get_type(ctx, &load->resource)); + regset = hlsl_type_get_regset(load->resource.data_type); if (!hlsl_regset_index_from_deref(ctx, &load->resource, regset, &index)) return false; diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 045fb6c5..731e9a72 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -3619,7 +3619,7 @@ static void sm4_register_from_deref(struct hlsl_ctx *ctx, struct sm4_register *r unsigned int *writemask, enum vkd3d_sm4_swizzle_type *swizzle_type, const struct hlsl_deref *deref) { - const struct hlsl_type *data_type = hlsl_deref_get_type(ctx, deref); + const struct hlsl_type *data_type = deref->data_type; const struct hlsl_ir_var *var = deref->var; if (var->is_uniform) @@ -4366,7 +4366,7 @@ static void write_sm4_ld(const struct tpf_writer *tpf, const struct hlsl_ir_node const struct hlsl_ir_node *sample_index, const struct hlsl_ir_node *texel_offset, enum hlsl_sampler_dim dim) { - const struct hlsl_type *resource_type = hlsl_deref_get_type(tpf->ctx, resource); + const struct hlsl_type *resource_type = resource->data_type; bool multisampled = resource_type->base_type == HLSL_TYPE_TEXTURE && (resource_type->sampler_dim == HLSL_SAMPLER_DIM_2DMS || resource_type->sampler_dim == HLSL_SAMPLER_DIM_2DMSARRAY); bool uav = (hlsl_type_get_regset(resource_type) == HLSL_REGSET_UAVS); @@ -5394,7 +5394,7 @@ static void write_sm4_resource_load(const struct tpf_writer *tpf, const struct h static void write_sm4_resource_store(const struct tpf_writer *tpf, const struct hlsl_ir_resource_store *store) { - struct hlsl_type *resource_type = hlsl_deref_get_type(tpf->ctx, &store->resource); + struct hlsl_type *resource_type = store->resource.data_type; if (!store->resource.var->is_uniform) { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.c:
{ memset(deref, 0, sizeof(*deref)); deref->var = var; Not a big deal and not really relevant to this MR, but I think it would make sense to call `init_deref()` here too.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343#note_46178
Yeah, I think this makes sense as it somewhat simplifies an internal interface, and possibly save a few cycles walking variable access chains. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343#note_46179
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343
I don't like this, sorry. I think deref->data_type is pretty definitely going to go away eventually. I'm not heavily opposed to shuffling things around in the meantime (although I don't see it as worthwhile). This does mean that 2/2 feels wrong, however. More importantly, I don't like 1/2. hlsl_deref_compute_data_type() specifically feels wrong, it turns a simple idempotent function into something that now has to be called every time we construct a deref. It's not really clear to me how the code gets simpler either, if it does. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343#note_47527
On Wed Oct 4 16:21:21 2023 +0000, Zebediah Figura wrote:
I don't like this, sorry. I think deref->data_type is pretty definitely going to go away eventually. I'm not heavily opposed to shuffling things around in the meantime (although I don't see it as worthwhile). This does mean that 2/2 feels wrong, however. More importantly, I don't like 1/2. hlsl_deref_compute_data_type() specifically feels wrong, it turns a simple idempotent function into something that now has to be called every time we construct a deref. It's not really clear to me how the code gets simpler either, if it does. I mean, we don't **need** to get rid of it even if we complete the translation to vsir output.
I think the biggest simplification is being able to just write `deref->data_type` instead of calling a function that also requires the ctx everywhere we need the data type of the deref. Still, I understand that this field is a derived attribute, and redundancy may lead us to forget we have to change things in two places if we want to do it, even though I can only think of a single scenario were we would change the type of a deref, that being merging separated samplers. Okay, I will write my rebasing of non-constant offset dereferences assuming that deref->data_type won't extend during the whole deref's lifetime. I will keep this MR open for a while though, in case there are additional comments. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343#note_47589
This merge request was closed by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/343
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)