Hi,
On 28-07-22 16:59, Matteo Bruni wrote:
On Wed, Jul 20, 2022 at 3:23 PM Francisco Casas fcasas@codeweavers.com wrote:
The transform_deref_paths_into_offsets pass turns these index paths back into register offsets.
Signed-off-by: Francisco Casas fcasas@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@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'.