[PATCH vkd3d v3 09/17] vkd3d-shader/hlsl: Replace register offsets with index paths in load initializations.
Matteo Bruni
matteo.mystral at gmail.com
Tue Aug 2 15:25:36 CDT 2022
On Tue, Aug 2, 2022 at 9:19 PM Francisco Casas <fcasas at codeweavers.com> wrote:
>
> Hi,
Hi Francisco,
big thumb up from me. I left a couple little replies in-line.
> On 28-07-22 16:59, Matteo Bruni wrote:
> > 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?
> >
>
> Yep.
>
> >> +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.
> >
>
> Okay, how about:
>
> /* Given a type and a component index, this function moves one step
> through the path required to
> * reach that component within the type.
> * It returns the first index of this path.
> * It sets *type_ptr to the (outermost) type within the original type
> that contains the component.
> * It sets *index_ptr to the index of the component within *type_ptr.
> * So, this function can be called several times in sequence to obtain
> all the path's indexes until
> * the component is finally reached. */
>
> ?
I like it.
> BTW, an alternative name for the function could be
> 'traverse_path_from_component_index', but I am not totally sure.
Also good, it feels like an improvement to me.
I wonder what Zeb and Giovanni think about this (if anything) :)
> >> +{
> >> + 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.
> >
>
> Okay, I will write it as:
>
> /* 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. */
>
> > 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.
> >
>
> I am for the "init_" prefix.
>
> > 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.
> >
>
> Hmm, while the other functions return pointers to nodes (or to a
> specific type of node) we never return derefs.
>
> Personally, when I see an initialization function that returns a
> pointer, my first inclination is to think that said function allocates
> memory, which isn't the case for derefs, which are most of time
> contained in other structs (hlsl_ir_store, hlsl_ir_load, or
> hlsl_ir_resource_load) and are never allocated separately.
Yes, that's a fair point.
>
> >> +{
> >> + 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.
>
> It makes sense to me to keep a distinction between 'new_' and 'init_'
> when the new value's memory is allocated in the function or not.
>
> Ok, I will rename it to 'hlsl_new_offset_instr_from_deref'.
More information about the wine-devel
mailing list