On Wed, Aug 3, 2022 at 11:26 PM Francisco Casas fcasas@codeweavers.com wrote:
On 28-07-22 16:44, Matteo Bruni wrote:
Okay, I guess I managed to read the patch series through. Sorry it took me way longer than I'd hoped, and I didn't even properly review the individual patches (e.g. apply them and check that the tests pass after each one).
In general the patch series seems okay to me and, as far as I'm concerned, if this goes in with no changes at all I would have no qualms.
That said, I do have a number of relatively minor comments that I'll send in reply to individual patches. For the earlier patches in the series (like this one) I'm going to reply to "v2" since that's where I had started to write them, but I don't think anything mentioned in those was affected by the changes in v3.
On Fri, Jul 15, 2022 at 3:24 AM Francisco Casas fcasas@codeweavers.com wrote:
Signed-off-by: Francisco Casas fcasas@codeweavers.com
v2:
- Use "const struct vkd3d_shader_location *" instead of "const struct vkd3d_shader_location".
- Removed 'in hlsl.y' in the patch subject.
- Use vkd3d_string_buffer for initializing the deref synthetic variable names.
- Move common "load = hlsl_new_load" pattern out of the if..else branches.
Signed-off-by: Francisco Casas fcasas@codeweavers.com
libs/vkd3d-shader/hlsl.y | 98 +++++++++++++++++++++++++++------------- 1 file changed, 67 insertions(+), 31 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index bfafd981..f5c9f4ca 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -272,8 +272,8 @@ static bool implicit_compatible_data_types(struct hlsl_type *t1, struct hlsl_typ return false; }
-static struct hlsl_ir_load *add_load(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_node,
struct hlsl_ir_node *offset, struct hlsl_type *data_type, const struct vkd3d_shader_location loc);+static struct hlsl_ir_load *add_load_component(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_node,
unsigned int comp, const struct vkd3d_shader_location *loc);static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *node, struct hlsl_type *dst_type, const struct vkd3d_shader_location *loc)
@@ -311,8 +311,8 @@ static struct hlsl_ir_node *add_cast(struct hlsl_ctx *ctx, struct list *instrs,
for (dst_idx = 0; dst_idx < dst_type->dimx * dst_type->dimy; ++dst_idx) {
struct hlsl_type *src_scalar_type, *dst_scalar_type;unsigned int src_idx, src_offset, dst_offset;
struct hlsl_type *dst_scalar_type;unsigned int src_idx, dst_offset; struct hlsl_ir_store *store; struct hlsl_ir_constant *c;@@ -335,13 +335,8 @@ 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);
src_offset = hlsl_compute_component_offset(ctx, src_type, src_idx, &src_scalar_type);if (!(c = hlsl_new_uint_constant(ctx, src_offset, loc)))return NULL;list_add_tail(instrs, &c->node.entry);if (!(load = add_load(ctx, instrs, node, &c->node, src_scalar_type, *loc)))
if (!(load = add_load_component(ctx, instrs, node, src_idx, loc))) return NULL; if (!(cast = hlsl_new_cast(ctx, &load->node, dst_scalar_type, loc)))@@ -668,6 +663,62 @@ static struct hlsl_ir_load *add_load(struct hlsl_ctx *ctx, struct list *instrs, return load; }
+static struct hlsl_ir_load *add_load_component(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_node,
unsigned int comp, const struct vkd3d_shader_location *loc)I would avoid using any "node" variable name in new code.
Understood. However, in this particular case (and later, add_load_index()) I can't think of a better name for the node argument, besides "value".
"Instr" should always work as a replacement. Node really isn't an appropriate name anymore, specifically since we flattened the IR a while back.