[PATCH vkd3d v3 09/17] vkd3d-shader/hlsl: Replace register offsets with index paths in load initializations.
Francisco Casas
fcasas at codeweavers.com
Tue Aug 2 14:18:57 CDT 2022
Hi,
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. */
?
BTW, an alternative name for the function could be
'traverse_path_from_component_index', but I am not totally sure.
>> +{
>> + 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.
>> +{
>> + 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