On 08-07-22 17:28, Zebediah Figura wrote:
On 7/1/22 16:24, Francisco Casas wrote:
+/* Returns the path of a given component within a type, given its index.
- *comp_type will be set to the type of the component.
- *path_len will be set to the lenght of the path.
- Memory should be free afterwards.
- */
+unsigned int *hlsl_compute_component_path(struct hlsl_ctx *ctx, struct hlsl_type *type, + unsigned int idx, struct hlsl_type **comp_type, unsigned int *path_len)
This is awkward partly because zero is a valid length. Ideally it should return bool. More than that, though, it's filling fields of struct hlsl_deref, so what about something like
static bool hlsl_deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_var *var, unsigned int index)
This obviously doesn't lend itself nicely to recursion. I ended up playing around with this for a bit and came up with the following abbreviated code:
unsigned int subtype_index_from_component_index(struct hlsl_ctx *ctx, inout const struct hlsl_type **type, inout unsigned int *index) { assert(index < hlsl_type_component_count(type));
... }
static bool hlsl_deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_var *var, uint index) { const struct hlsl_type *type = var->data_type; unsigned int *indices; size_t index_count;
while (/* type is not 1x1 numeric */) { indices[index_count++] = subtype_index_from_component(ctx, &type, &index); }
init_deref(ctx, deref, var, index_count); // and so on }
Thoughts?
At first glance, and assuming we don't need hlsl_compute_component_path() somewhere else, this seems like a great idea!
I could also use subtype_index_from_component_index() to create a function to retrieve the component type without retrieving the path. As this is needed a couple of times.
+/* Returns a simple variable derefence, so that the value can be stored and then passed by reference
- to load/store functions. It shall not be modified afterwards. */
+struct hlsl_deref hlsl_get_direct_var_deref(struct hlsl_ir_var *var) +{ + struct hlsl_deref deref = {};
+ deref.var = var; + return deref; +}
Naming: for consistency with hlsl_new_simple_store() I'd replace "direct" with "simple". (Also naming: perhaps hlsl_simple_deref_from_var instead, i.e. follow the "X from Y" pattern given what the function is doing).
I'm also inclined to say this should take the hlsl_deref as a pointer in the first argument, rather than returning a struct.
Okay.
struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, struct hlsl_type *data_type, - enum hlsl_resource_load_type type, struct hlsl_ir_var *resource, struct hlsl_ir_node *resource_offset, - struct hlsl_ir_var *sampler, struct hlsl_ir_node *sampler_offset, struct hlsl_ir_node *coords, - struct hlsl_ir_node *texel_offset, const struct vkd3d_shader_location *loc) + enum hlsl_resource_load_type type, struct hlsl_deref *resource, struct hlsl_deref *sampler, + struct hlsl_ir_node *coords, struct hlsl_ir_node *texel_offset, const struct vkd3d_shader_location *loc)
Separate patch?
Hmm, I will give it a try.
@@ -870,10 +1136,8 @@ struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, struc return NULL; init_node(&load->node, HLSL_IR_RESOURCE_LOAD, data_type, *loc); load->load_type = type; - load->resource.var = resource; - hlsl_src_from_node(&load->resource.offset, resource_offset); - load->sampler.var = sampler; - hlsl_src_from_node(&load->sampler.offset, sampler_offset); + deref_copy(ctx, &load->resource, resource); + deref_copy(ctx, &load->sampler, sampler);
Actually that means deref_copy() could be pulled out of this patch, right? Just to save another hunk :-)
Sure!
@@ -334,7 +336,10 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, } } - dst_offset = hlsl_compute_component_offset(ctx, dst_type, dst_idx, &dst_scalar_type); + dst_path = hlsl_compute_component_path(ctx, dst_type, dst_idx, &dst_scalar_type, &dst_path_len); + if (dst_path_len && !dst_path) + return NULL; + vkd3d_free(dst_path);
As far as I can tell we're just doing this so that we can get the right destination type to cast into. In that case my inclination is to move that cast into hlsl_new_store_component() itself. Same in initialize_var_components().
We could also use the aforementioned hypothetical function that retrieves the component type without retrieving the path. I will evaluate both alternatives.
if (!(load = add_load_component(ctx, instrs, node, src_idx, *loc))) return NULL; @@ -343,16 +348,12 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, return NULL; list_add_tail(instrs, &cast->node.entry); - if (!(c = hlsl_new_uint_constant(ctx, dst_offset, loc))) + if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, dst_idx, &cast->node))) return NULL; - list_add_tail(instrs, &c->node.entry);
- if (!(store = hlsl_new_store(ctx, var, &c->node, &cast->node, 0, *loc))) - return NULL; - list_add_tail(instrs, &store->node.entry); + list_move_tail(instrs, &block.instrs); } - if (!(load = hlsl_new_load(ctx, var, NULL, dst_type, *loc))) + if (!(load = hlsl_new_var_load(ctx, var, *loc))) return NULL; list_add_tail(instrs, &load->node.entry);
...
@@ -625,22 +626,13 @@ static struct hlsl_ir_jump *add_return(struct hlsl_ctx *ctx, struct list *instrs static struct hlsl_ir_load *add_load_index(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_node, struct hlsl_ir_node *idx, const struct vkd3d_shader_location loc) { - struct hlsl_type *elem_type; - struct hlsl_ir_node *offset; struct hlsl_ir_load *load; - struct hlsl_block block;
- elem_type = hlsl_get_type_from_path_index(ctx, var_node->data_type, idx); if (var_node->type == HLSL_IR_LOAD) { const struct hlsl_deref *src = &hlsl_ir_load(var_node)->src; - if (!(offset = hlsl_new_offset_from_path_index(ctx, &block, var_node->data_type, src->offset.node, idx, &loc))) - return NULL; - list_move_tail(instrs, &block.instrs);
- if (!(load = hlsl_new_load(ctx, src->var, offset, elem_type, loc))) + if (!(load = hlsl_new_load_index(ctx, src, idx, loc))) return NULL; list_add_tail(instrs, &load->node.entry); }
Is it possible to introduce hlsl_new_load_index() and hlsl_new_load_component() in helper patch(es), the same way we did for add_load_index()? Same for stores I think.
(I know this is a lot of splitting, and/or busywork, but the more the patch is split, the easier it is to be sure we've caught all the errors.)
I fear I may need to add a lot of transitional code, which also requires to be understood, to keep the program working, so this splitting seems to be an "economy of scale" thing. Right now I am not sure how much of this transitional code would be actually needed. I will give it a try.