[PATCH vkd3d v3 09/17] vkd3d-shader/hlsl: Replace register offsets with index paths in load initializations.
Matteo Bruni
matteo.mystral at gmail.com
Thu Jul 28 15:59:14 CDT 2022
On Wed, Jul 20, 2022 at 3:23 PM Francisco Casas <fcasas at codeweavers.com> wrote:
>
> The transform_deref_paths_into_offsets pass turns these index paths back
> into register offsets.
>
> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> ---
>
> The idea is that we can move the transform_deref_paths_into_offsets()
> pass forward as we translate more passes to work with index paths.
> This, until register offsets can be totally removed, after we implement
> the SMxIRs and their translations.
>
> The aim is to have 3 ways of initializing load/store nodes when using index
> paths:
>
> * One that initializes the node from a another's node deref and and
> optional index to be appended to that deref's path.
> * One that initializes the node from a deref and the index
> of a single basic component within it. This one also generates constant
> nodes for the required path, so it also initializes an instruction block
> whose instructions must be inserted in the instruction list.
> * One that initializes the node directly for a whole variable. These functions
> are already present: hlsl_new_var_load() and hlsl_new_simple_store().
>
> The signatures of these functions are to be placed nearby in hlsl.h as
> they are introduced in the following patches.
>
> It is worth noting that the use of index paths allows to remove the data type
> argument when initializing store/loads because it can now be deducted from the
> variable and the hlsl_deref.
>
> Applying an index over a matrix derefence retrieves a vector. If the matrix
> is row_major, this corresponds to a row, otherwise, it corresponds to a
> column. So, the code should take matrix majority into account, at least until
> the split_matrix_copies pass.
>
> The first index in a path after a loading a struct should be an
> hlsl_ir_constant, since the field that's being addressed is always
> known at parse-time.
>
> hlsl_init_simple_deref_from_var() can be used to initialize a deref that can
> be passed by reference to the load and store initialization functions.
> This value shall not be modified after being created and does not
> require to call hlsl_cleanup_deref().
> The deref obtained with this function, can also be passed be passed as prefix
> to deref_from_component_index().
>
> ---
>
> v3:
> * Replaced compute_component_path() with deref_from_component_index().
> * Wrote implementation of init_deref() and get_type_from_deref() further
> up in the file.
> * Made hlsl_new_load_component() use deref_from_component_index()
> instead of the removed compute_component_path().
> * Rewrote hlsl.c function comments in present and active voice.
> * Renamed
> typep -> type_ptr
> indexp -> index_ptr
> in subtype_index_from_component_index().
> * Added space before '?' in ternary operators.
>
> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> ---
> libs/vkd3d-shader/hlsl.c | 298 +++++++++++++++++++++++++++++--
> libs/vkd3d-shader/hlsl.h | 35 +++-
> libs/vkd3d-shader/hlsl.y | 102 ++++++-----
> libs/vkd3d-shader/hlsl_codegen.c | 44 +++++
> 4 files changed, 410 insertions(+), 69 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> index 66acce23..535433ee 100644
> --- a/libs/vkd3d-shader/hlsl.c
> +++ b/libs/vkd3d-shader/hlsl.c
> @@ -330,6 +330,176 @@ unsigned int hlsl_compute_component_offset(struct hlsl_ctx *ctx, struct hlsl_typ
> return 0;
> }
>
> +static bool type_is_single_component(const struct hlsl_type *type)
> +{
> + return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_OBJECT;
> +}
> +
> +/* Given a type and a component index, this function returns the next path index required to reach
> + * the component within the type.
> + * It sets *type to the subtype within the original type that contains the component.
> + * It sets *index to the index of the component within *type. */
Now *type_ptr and *index_ptr respectively, right?
> +static unsigned int subtype_index_from_component_index(struct hlsl_ctx *ctx,
> + struct hlsl_type **type_ptr, unsigned int *index_ptr)
After rereading the patch a number of times, I think the prototype is
okay (not sure about the name, it sounds like a "get" but it does
change the two parameters, not that I have better suggestions) but
that comment above needs some clarification. What this function does
is to move one step through the "path", or component index I guess,
returning the "index" to be taken for the outer data type and updating
type_ptr and index_ptr (which are two in/out parameters btw, their
types do make it somewhat expected but I wouldn't say it's super
obvious at a first glance) with the next inner type and index.
Which I guess is what the comment is trying to convey, but dunno, it
really didn't work for me. It might be enough to tweak it with more
details (e.g. next -> outer?) or maybe it deserves some more
substantial rewriting.
> +{
> + struct hlsl_type *type = *type_ptr;
> + unsigned int index = *index_ptr;
> +
> + assert(!type_is_single_component(type));
> + assert(index < hlsl_type_component_count(type));
> +
> + switch (type->type)
> + {
> + case HLSL_CLASS_VECTOR:
> + assert(index < type->dimx);
> + *type_ptr = hlsl_get_scalar_type(ctx, type->base_type);
> + *index_ptr = 0;
> + return index;
> +
> + case HLSL_CLASS_MATRIX:
> + {
> + unsigned int y = index / type->dimx, x = index % type->dimx;
> + bool row_major = hlsl_type_is_row_major(type);
> +
> + assert(index < type->dimx * type->dimy);
> + *type_ptr = hlsl_get_vector_type(ctx, type->base_type, row_major ? type->dimx : type->dimy);
> + *index_ptr = row_major ? x : y;
> + return row_major ? y : x;
> + }
> +
> + case HLSL_CLASS_ARRAY:
> + {
> + unsigned int elem_comp_count = hlsl_type_component_count(type->e.array.type);
> + unsigned int array_index;
> +
> + *type_ptr = type->e.array.type;
> + *index_ptr = index % elem_comp_count;
> + array_index = index / elem_comp_count;
> + assert(array_index < type->e.array.elements_count);
> + return array_index;
> + }
> +
> + case HLSL_CLASS_STRUCT:
> + {
> + struct hlsl_struct_field *field;
> + unsigned int field_comp_count, i;
> +
> + for (i = 0; i < type->e.record.field_count; ++i)
> + {
> + field = &type->e.record.fields[i];
> + field_comp_count = hlsl_type_component_count(field->type);
> + if (index < field_comp_count)
> + {
> + *type_ptr = field->type;
> + *index_ptr = index;
> + return i;
> + }
> + index -= field_comp_count;
> + }
> + assert(0);
> + return 0;
> + }
> +
> + default:
> + assert(0);
> + return 0;
> + }
> +}
> +
> +struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type,
> + unsigned int index)
> +{
> + while (!type_is_single_component(type))
> + subtype_index_from_component_index(ctx, &type, &index);
> +
> + return type;
> +}
> +
> +static bool init_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_var *var,
> + unsigned int path_len)
> +{
> + deref->var = var;
> + deref->path_len = path_len;
> + deref->offset.node = NULL;
> +
> + if (path_len == 0)
> + {
> + deref->path = NULL;
> + return true;
> + }
> +
> + if (!(deref->path = hlsl_alloc(ctx, sizeof(*deref->path) * deref->path_len)))
> + {
> + deref->var = NULL;
> + deref->path_len = 0;
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static struct hlsl_type *get_type_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref)
> +{
> + struct hlsl_type *type;
> + unsigned int i;
> +
> + assert(deref);
> + assert(!deref->offset.node);
> +
> + type = deref->var->data_type;
> + for (i = 0; i < deref->path_len; ++i)
> + type = hlsl_get_type_from_path_index(ctx, type, deref->path[i].node);
> + return type;
> +}
> +
> +/* Initializes a deref from another deref (prefix) and a component index. */
> +static bool deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_block *block,
> + struct hlsl_deref *deref, const struct hlsl_deref *prefix, unsigned int index,
> + const struct vkd3d_shader_location *loc)
It might come as a shock but I don't like this function's name.
Originally these x_from_y() kind of functions were always "conversion"
functions, i.e. take object y and return something from it as type x.
Which I guess is technically what this function is, but I don't feel
like that is the best way to represent it.
For one, this function effectively has 2 out parameters (block and
deref), in that it also generates instructions and adds them to the
block list. I guess it's not any different from a few other functions
you're introducing in the patchset but, as it is, the comment feels
somewhat partial. I would mention "block" as another byproduct of the
function in the comment at the top.
In regard to the name, maybe just adding a new_ (or create_?
generate_? init_?) in front would be enough. I don't have great
suggestions, as usual.
Somewhat minor, and probably more controversial, but maybe still worth
mentioning: instead of returning a bool, the function could return a
pointer to the deref. It should return NULL in the codepaths currently
returning "false", to maintain the current semantics (which are fine).
While it's unlikely that the return value is going to be super useful,
making this function resemble the other similar ones introduced in
this patch and previously has value in my mind. Also, of course, that
doesn't mean you can drop the "deref" parameter.
> +{
> + unsigned int path_len, path_index, deref_path_len, i;
> + struct hlsl_type *path_type;
> + struct hlsl_ir_constant *c;
> +
> + list_init(&block->instrs);
> +
> + path_len = 0;
> + path_type = get_type_from_deref(ctx, prefix);
> + path_index = index;
> + while (!type_is_single_component(path_type))
> + {
> + subtype_index_from_component_index(ctx, &path_type, &path_index);
> + ++path_len;
> + }
> +
> + if (!init_deref(ctx, deref, prefix->var, prefix->path_len + path_len))
> + return false;
> +
> + deref_path_len = 0;
> + for (i = 0; i < prefix->path_len; ++i)
> + hlsl_src_from_node(&deref->path[deref_path_len++], prefix->path[i].node);
> +
> + path_type = get_type_from_deref(ctx, prefix);
> + path_index = index;
> + while (!type_is_single_component(path_type))
> + {
> + unsigned int next_index = subtype_index_from_component_index(ctx, &path_type, &path_index);
> +
> + if (!(c = hlsl_new_uint_constant(ctx, next_index, loc)))
> + {
> + hlsl_free_instr_list(&block->instrs);
> + return false;
> + }
> + list_add_tail(&block->instrs, &c->node.entry);
> +
> + hlsl_src_from_node(&deref->path[deref_path_len++], &c->node);
> + }
> +
> + assert(deref_path_len == deref->path_len);
> +
> + return true;
> +}
> +
> struct hlsl_type *hlsl_get_type_from_path_index(struct hlsl_ctx *ctx, const struct hlsl_type *type,
> struct hlsl_ir_node *node)
> {
> @@ -435,6 +605,37 @@ struct hlsl_ir_node *hlsl_new_offset_from_path_index(struct hlsl_ctx *ctx, struc
> return idx_offset;
> }
>
> +struct hlsl_ir_node *hlsl_new_offset_node_from_deref(struct hlsl_ctx *ctx, struct hlsl_block *block,
> + const struct hlsl_deref *deref, const struct vkd3d_shader_location *loc)
For reference, I find the naming of this one okay (aside from node ->
instr or something), probably just because of the new_ prefix.
More information about the wine-devel
mailing list