[PATCH v2 0/7] MR124: vkd3d-shader/hlsl: Introduce hlsl_ir_index, part 1/2.
This node type is intended for use during parse-time. While we parse an indexing expression such as `a[3]`, we don't know if it will end up as part of an expression (in which case it must be folded into a load) or it is for the lhs of a store (in which case it must be folded into the store's deref). This node type is used to represent these accesses and no longer rely on building an `hlsl_ir_load` for each array index or struct record access. `hlsl_ir_index` chains are lowered into derefs when (and if) they are used to specify the lhs of an assignment. All `hlsl_ir_index`es are lowered into `hlsl_ir_load`s with a compilation pass. The changes introduced in these series allow to solve the problem with the return variable of function calls presented in !93, and to properly support assignment to matrix indexes, which is something we are not doing correctly. Further patches (in my [index node](https://gitlab.winehq.org/fcasas/vkd3d/-/commits/index_node) branch) add support for indexing non-load expressions, such as `(a + b)[1]` and allowing to represent resource loads through `hlsl_ir_index`, so that `hlsl_ir_resource_store`s don't have to rely on `hlsl_ir_resource_load`s. -- v2: vkd3d-shader/hlsl: Support matrix indexing in the lhs. vkd3d-shader/hlsl: Always load from a synthetic copy in add_load_component(). vkd3d-shader/hlsl: Remove add_load_index(). vkd3d-shader/hlsl: Use hlsl_ir_index for array and record access. vkd3d-shader/hlsl: Introduce hlsl_ir_index. tests: Test matrix indexing on the lhs. tests: Test multiple calls to the same function in initializers. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/hlsl-function.shader_test | 34 +++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/hlsl-function.shader_test b/tests/hlsl-function.shader_test index c836808d..a7cad6d7 100644 --- a/tests/hlsl-function.shader_test +++ b/tests/hlsl-function.shader_test @@ -261,3 +261,37 @@ float4 main() : sv_target func(true); return 0; } + + +[pixel shader] +float func(float a) +{ + return a + 1; +} + +float4 main() : sv_target +{ + return float4(func(1.0), func(2.0), func(5.0), func(6.0)); +} + +[test] +draw quad +todo probe all rgba (2.0, 3.0, 6.0, 7.0) + + +[pixel shader] +float func(float a) +{ + return a + 1; +} + +float4 main() : sv_target +{ + float4 a = {func(1.0), func(2.0), func(5.0), func(6.0)}; + + return a; +} + +[test] +draw quad +todo probe all rgba (2.0, 3.0, 6.0, 7.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/hlsl-matrix-indexing.shader_test | 62 ++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/tests/hlsl-matrix-indexing.shader_test b/tests/hlsl-matrix-indexing.shader_test index 716b43d4..7ae1a91d 100644 --- a/tests/hlsl-matrix-indexing.shader_test +++ b/tests/hlsl-matrix-indexing.shader_test @@ -76,3 +76,65 @@ float4 main() : SV_TARGET [test] draw quad probe all rgba (1.0, 5.0, 7.0, 12.0) + + +[pixel shader] +float4 main() : SV_TARGET +{ + float3x2 m = {1, 2, 3, 4, 5, 6}; + + m[1] = float2(30, 40); + + return float4(m[1], m[2]); +} + +[test] +draw quad +todo probe all rgba (30.0, 40.0, 5.0, 6.0) + + +[pixel shader] +float4 main() : SV_TARGET +{ + row_major float3x2 m = {1, 2, 3, 4, 5, 6}; + + m[2] = float2(50, 60); + + return float4(m[1], m[2]); +} + +[test] +draw quad +probe all rgba (3.0, 4.0, 50.0, 60.0) + + +[pixel shader todo] +uniform int i; + +float4 main() : sv_target +{ + float4x4 mat = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; + + return mat[i]; +} + +[test] +uniform 0 int 2 +todo draw quad +todo probe all rgba (8, 9, 10, 11) + + +[pixel shader todo] +uniform int i; + +float4 main() : sv_target +{ + row_major float4x4 mat = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}; + + return mat[i]; +} + +[test] +uniform 0 int 3 +todo draw quad +todo probe all rgba (12, 13, 14, 15) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124
From: Francisco Casas <fcasas(a)codeweavers.com> This node type is intended for use during parse-time. While we parse an indexing expression such as "a[3]", we don't know if it will end up as part of an expression (in which case it must be folded into a load) or it is for the lhs of a store (in which case it must be folded into the store's deref). --- This patch adds what may be considered dead-code, but this is promptly used in the following patch. I organized it this way to somewhat ease the review. --- libs/vkd3d-shader/hlsl.c | 67 ++++++++++++++++++++++++++++++++ libs/vkd3d-shader/hlsl.h | 17 ++++++++ libs/vkd3d-shader/hlsl.y | 1 + libs/vkd3d-shader/hlsl_codegen.c | 9 +++++ 4 files changed, 94 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 24822e97..2b89660a 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1274,6 +1274,35 @@ struct hlsl_ir_swizzle *hlsl_new_swizzle(struct hlsl_ctx *ctx, DWORD s, unsigned return swizzle; } +bool hlsl_index_is_crooked_matrix_indexing(struct hlsl_ir_index *index) +{ + struct hlsl_type *type = index->val.node->data_type; + + return type->type == HLSL_CLASS_MATRIX && !hlsl_type_is_row_major(type); +} + +struct hlsl_ir_index *hlsl_new_index(struct hlsl_ctx *ctx, struct hlsl_ir_node *val, + struct hlsl_ir_node *idx, const struct vkd3d_shader_location *loc) +{ + struct hlsl_type *type = val->data_type; + struct hlsl_ir_index *index; + + if (!(index = hlsl_alloc(ctx, sizeof(*index)))) + return NULL; + + if (type->type == HLSL_CLASS_OBJECT) + type = type->e.resource_format; + else if (type->type == HLSL_CLASS_MATRIX) + type = hlsl_get_vector_type(ctx, type->base_type, type->dimx); + else + type = hlsl_get_element_type_from_path_index(ctx, type, idx); + + init_node(&index->node, HLSL_IR_INDEX, type, loc); + hlsl_src_from_node(&index->val, val); + hlsl_src_from_node(&index->idx, idx); + return index; +} + struct hlsl_ir_jump *hlsl_new_jump(struct hlsl_ctx *ctx, enum hlsl_ir_jump_type type, struct vkd3d_shader_location loc) { struct hlsl_ir_jump *jump; @@ -1533,6 +1562,17 @@ static struct hlsl_ir_node *clone_swizzle(struct hlsl_ctx *ctx, return &dst->node; } +static struct hlsl_ir_node *clone_index(struct hlsl_ctx *ctx, struct clone_instr_map *map, + struct hlsl_ir_index *src) +{ + struct hlsl_ir_index *dst; + + if (!(dst = hlsl_new_index(ctx, map_instr(map, src->val.node), map_instr(map, src->idx.node), + &src->node.loc))) + return NULL; + return &dst->node; +} + static struct hlsl_ir_node *clone_instr(struct hlsl_ctx *ctx, struct clone_instr_map *map, const struct hlsl_ir_node *instr) { @@ -1570,6 +1610,9 @@ static struct hlsl_ir_node *clone_instr(struct hlsl_ctx *ctx, case HLSL_IR_SWIZZLE: return clone_swizzle(ctx, map, hlsl_ir_swizzle(instr)); + + case HLSL_IR_INDEX: + return clone_index(ctx, map, hlsl_ir_index(instr)); } vkd3d_unreachable(); @@ -1946,6 +1989,7 @@ const char *hlsl_node_type_to_string(enum hlsl_ir_node_type type) "HLSL_IR_RESOURCE_STORE", "HLSL_IR_STORE", "HLSL_IR_SWIZZLE", + "HLSL_IR_INDEX", }; if (type >= ARRAY_SIZE(names)) @@ -2317,6 +2361,14 @@ static void dump_ir_swizzle(struct vkd3d_string_buffer *buffer, const struct hls } } +static void dump_ir_index(struct vkd3d_string_buffer *buffer, const struct hlsl_ir_index *index) +{ + dump_src(buffer, &index->val); + vkd3d_string_buffer_printf(buffer, "[idx:"); + dump_src(buffer, &index->idx); + vkd3d_string_buffer_printf(buffer, "]"); +} + static void dump_instr(struct hlsl_ctx *ctx, struct vkd3d_string_buffer *buffer, const struct hlsl_ir_node *instr) { if (instr->index) @@ -2371,6 +2423,10 @@ static void dump_instr(struct hlsl_ctx *ctx, struct vkd3d_string_buffer *buffer, case HLSL_IR_SWIZZLE: dump_ir_swizzle(buffer, hlsl_ir_swizzle(instr)); break; + + case HLSL_IR_INDEX: + dump_ir_index(buffer, hlsl_ir_index(instr)); + break; } } @@ -2518,6 +2574,13 @@ static void free_ir_swizzle(struct hlsl_ir_swizzle *swizzle) vkd3d_free(swizzle); } +static void free_ir_index(struct hlsl_ir_index *index) +{ + hlsl_src_remove(&index->val); + hlsl_src_remove(&index->idx); + vkd3d_free(index); +} + void hlsl_free_instr(struct hlsl_ir_node *node) { assert(list_empty(&node->uses)); @@ -2567,6 +2630,10 @@ void hlsl_free_instr(struct hlsl_ir_node *node) case HLSL_IR_SWIZZLE: free_ir_swizzle(hlsl_ir_swizzle(node)); break; + + case HLSL_IR_INDEX: + free_ir_index(hlsl_ir_index(node)); + break; } } diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 24d0f517..b16c7b80 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -259,6 +259,7 @@ enum hlsl_ir_node_type HLSL_IR_RESOURCE_STORE, HLSL_IR_STORE, HLSL_IR_SWIZZLE, + HLSL_IR_INDEX, }; /* Common data for every type of IR instruction node. */ @@ -538,6 +539,12 @@ struct hlsl_ir_swizzle DWORD swizzle; }; +struct hlsl_ir_index +{ + struct hlsl_ir_node node; + struct hlsl_src val, idx; +}; + /* Reference to a variable, or a part of it (e.g. a vector within a matrix within a struct). */ struct hlsl_deref { @@ -848,6 +855,12 @@ static inline struct hlsl_ir_swizzle *hlsl_ir_swizzle(const struct hlsl_ir_node return CONTAINING_RECORD(node, struct hlsl_ir_swizzle, node); } +static inline struct hlsl_ir_index *hlsl_ir_index(const struct hlsl_ir_node *node) +{ + assert(node->type == HLSL_IR_INDEX); + return CONTAINING_RECORD(node, struct hlsl_ir_index, node); +} + static inline void hlsl_src_from_node(struct hlsl_src *src, struct hlsl_ir_node *node) { src->node = node; @@ -1048,6 +1061,10 @@ struct hlsl_ir_store *hlsl_new_store_index(struct hlsl_ctx *ctx, const struct hl struct hlsl_ir_store *hlsl_new_store_component(struct hlsl_ctx *ctx, struct hlsl_block *block, const struct hlsl_deref *lhs, unsigned int comp, struct hlsl_ir_node *rhs); +bool hlsl_index_is_crooked_matrix_indexing(struct hlsl_ir_index *index); + +struct hlsl_ir_index *hlsl_new_index(struct hlsl_ctx *ctx, struct hlsl_ir_node *val, + struct hlsl_ir_node *idx, const struct vkd3d_shader_location *loc); struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, struct vkd3d_shader_location loc); struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, const struct hlsl_resource_load_params *params, const struct vkd3d_shader_location *loc); diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index e011ea24..7435ef10 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1156,6 +1156,7 @@ static unsigned int evaluate_array_dimension(struct hlsl_ir_node *node) case HLSL_IR_LOAD: case HLSL_IR_RESOURCE_LOAD: case HLSL_IR_SWIZZLE: + case HLSL_IR_INDEX: FIXME("Unhandled type %s.\n", hlsl_node_type_to_string(node->type)); return 0; diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 65bd9d4c..553aedef 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2158,6 +2158,7 @@ static bool dce(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) case HLSL_IR_LOAD: case HLSL_IR_RESOURCE_LOAD: case HLSL_IR_SWIZZLE: + case HLSL_IR_INDEX: if (list_empty(&instr->uses)) { list_remove(&instr->entry); @@ -2394,6 +2395,14 @@ static void compute_liveness_recurse(struct hlsl_block *block, unsigned int loop swizzle->val.node->last_read = instr->index; break; } + case HLSL_IR_INDEX: + { + struct hlsl_ir_index *index = hlsl_ir_index(instr); + + index->val.node->last_read = instr->index; + index->idx.node->last_read = instr->index; + break; + } case HLSL_IR_CONSTANT: case HLSL_IR_JUMP: break; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124
From: Francisco Casas <fcasas(a)codeweavers.com>
From this point on, it is no longer true that only hlsl_ir_loads can return objects, because an object can also come from chain of hlsl_ir_indexes that ends in an hlsl_ir_load.
For this reason, hlsl_resource_load_params now expects both the resource as the sampler to be just an hlsl_ir_node pointer instead of a pointer to a more specific hlsl_ir_load. --- libs/vkd3d-shader/hlsl.c | 72 +++++++++++++- libs/vkd3d-shader/hlsl.h | 3 +- libs/vkd3d-shader/hlsl.y | 128 ++++++++----------------- libs/vkd3d-shader/hlsl_codegen.c | 99 +++++++++++++++++++ tests/hlsl-matrix-indexing.shader_test | 4 +- 5 files changed, 211 insertions(+), 95 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 2b89660a..79f85a06 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -435,6 +435,71 @@ static bool init_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hl return true; } +bool hlsl_init_deref_from_index_chain(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_node *chain) +{ + struct hlsl_ir_index *index; + struct hlsl_ir_load *load; + unsigned int chain_len, i; + struct hlsl_ir_node *ptr; + + deref->path = NULL; + deref->path_len = 0; + deref->offset.node = NULL; + + if (!chain) + return true; + + if (chain->type == HLSL_IR_INDEX) + assert(!hlsl_index_is_crooked_matrix_indexing(hlsl_ir_index(chain))); + + /* Find the length of the index chain */ + chain_len = 0; + ptr = chain; + while (ptr->type == HLSL_IR_INDEX) + { + index = hlsl_ir_index(ptr); + chain_len++; + ptr = index->val.node; + } + + if (ptr->type != HLSL_IR_LOAD) + { + /* This should be an hlsl_error() for invalid l-values, but indexing non-load expressions is + * also not supported yet so we use a hlsl_fixme() for now. */ + hlsl_fixme(ctx, &chain->loc, "Indexing %s.", hlsl_node_type_to_string(ptr->type)); + return false; + } + + load = hlsl_ir_load(ptr); + + if (!init_deref(ctx, deref, load->src.var, load->src.path_len + chain_len)) + return false; + + for (i = 0; i < load->src.path_len; ++i) + hlsl_src_from_node(&deref->path[i], load->src.path[i].node); + + ptr = chain; + for (i = 0; i < chain_len; ++i) + { + unsigned int p = deref->path_len - 1 - i; + + index = hlsl_ir_index(ptr); + if (!hlsl_index_is_crooked_matrix_indexing(index)) + { + hlsl_src_from_node(&deref->path[p], index->idx.node); + } + else + { + hlsl_src_from_node(&deref->path[p], deref->path[p + 1].node); + hlsl_src_remove(&deref->path[p + 1]); + hlsl_src_from_node(&deref->path[p + 1], index->idx.node); + } + ptr = index->val.node; + } + + return true; +} + struct hlsl_type *hlsl_deref_get_type(struct hlsl_ctx *ctx, const struct hlsl_deref *deref) { struct hlsl_type *type; @@ -1225,17 +1290,20 @@ struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, return NULL; init_node(&load->node, HLSL_IR_RESOURCE_LOAD, params->format, loc); load->load_type = params->type; - if (!hlsl_copy_deref(ctx, &load->resource, ¶ms->resource)) + + if (!hlsl_init_deref_from_index_chain(ctx, &load->resource, params->resource)) { vkd3d_free(load); return NULL; } - if (!hlsl_copy_deref(ctx, &load->sampler, ¶ms->sampler)) + + if (!hlsl_init_deref_from_index_chain(ctx, &load->sampler, params->sampler)) { hlsl_cleanup_deref(&load->resource); vkd3d_free(load); return NULL; } + hlsl_src_from_node(&load->coords, params->coords); hlsl_src_from_node(&load->texel_offset, params->texel_offset); hlsl_src_from_node(&load->lod, params->lod); diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index b16c7b80..05f162dd 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -785,7 +785,7 @@ struct hlsl_resource_load_params { struct hlsl_type *format; enum hlsl_resource_load_type type; - struct hlsl_deref resource, sampler; + struct hlsl_ir_node *resource, *sampler; struct hlsl_ir_node *coords, *lod, *texel_offset; }; @@ -997,6 +997,7 @@ void hlsl_dump_function(struct hlsl_ctx *ctx, const struct hlsl_ir_function_decl int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_func, enum vkd3d_shader_target_type target_type, struct vkd3d_shader_code *out); +bool hlsl_init_deref_from_index_chain(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_node *chain); bool hlsl_copy_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, const struct hlsl_deref *other); void hlsl_cleanup_deref(struct hlsl_deref *deref); diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 7435ef10..4c64ef86 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -685,9 +685,10 @@ static struct hlsl_ir_load *add_load_component(struct hlsl_ctx *ctx, struct list return load; } -static bool add_record_load(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *record, +static bool add_record_access(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *record, unsigned int idx, const struct vkd3d_shader_location loc) { + struct hlsl_ir_index *index; struct hlsl_ir_constant *c; assert(idx < record->data_type->e.record.field_count); @@ -696,60 +697,17 @@ static bool add_record_load(struct hlsl_ctx *ctx, struct list *instrs, struct hl return false; list_add_tail(instrs, &c->node.entry); - return !!add_load_index(ctx, instrs, record, &c->node, &loc); + if (!(index = hlsl_new_index(ctx, record, &c->node, &loc))) + return false; + list_add_tail(instrs, &index->node.entry); + + return true; } static struct hlsl_ir_node *add_binary_arithmetic_expr(struct hlsl_ctx *ctx, struct list *instrs, enum hlsl_ir_expr_op op, struct hlsl_ir_node *arg1, struct hlsl_ir_node *arg2, const struct vkd3d_shader_location *loc); -static bool add_matrix_index(struct hlsl_ctx *ctx, struct list *instrs, - struct hlsl_ir_node *matrix, struct hlsl_ir_node *index, const struct vkd3d_shader_location *loc) -{ - struct hlsl_type *mat_type = matrix->data_type, *ret_type; - struct hlsl_deref var_deref; - struct hlsl_ir_load *load; - struct hlsl_ir_var *var; - unsigned int i; - - if (hlsl_type_is_row_major(mat_type)) - return add_load_index(ctx, instrs, matrix, index, loc); - - ret_type = hlsl_get_vector_type(ctx, mat_type->base_type, mat_type->dimx); - - if (!(var = hlsl_new_synthetic_var(ctx, "index", ret_type, loc))) - return false; - hlsl_init_simple_deref_from_var(&var_deref, var); - - for (i = 0; i < mat_type->dimx; ++i) - { - struct hlsl_ir_load *column, *value; - struct hlsl_ir_store *store; - struct hlsl_ir_constant *c; - struct hlsl_block block; - - if (!(c = hlsl_new_uint_constant(ctx, i, loc))) - return false; - list_add_tail(instrs, &c->node.entry); - - if (!(column = add_load_index(ctx, instrs, matrix, &c->node, loc))) - return false; - - if (!(value = add_load_index(ctx, instrs, &column->node, index, loc))) - return false; - - if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, i, &value->node))) - return false; - list_move_tail(instrs, &block.instrs); - } - - if (!(load = hlsl_new_var_load(ctx, var, *loc))) - return false; - list_add_tail(instrs, &load->node.entry); - - return true; -} - static struct hlsl_ir_node *add_zero_mipmap_level(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *index, unsigned int dim_count, const struct vkd3d_shader_location *loc) { @@ -783,10 +741,11 @@ static struct hlsl_ir_node *add_zero_mipmap_level(struct hlsl_ctx *ctx, struct l return &coords_load->node; } -static bool add_array_load(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *array, +static bool add_array_access(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *array, struct hlsl_ir_node *index, const struct vkd3d_shader_location *loc) { const struct hlsl_type *expr_type = array->data_type, *index_type = index->data_type; + struct hlsl_ir_index *return_index; struct hlsl_ir_expr *cast; if (expr_type->type == HLSL_CLASS_OBJECT @@ -795,8 +754,6 @@ static bool add_array_load(struct hlsl_ctx *ctx, struct list *instrs, struct hls { struct hlsl_resource_load_params load_params = {.type = HLSL_RESOURCE_LOAD}; unsigned int dim_count = hlsl_sampler_dim_count(expr_type->sampler_dim); - /* Only HLSL_IR_LOAD can return an object. */ - struct hlsl_ir_load *object_load = hlsl_ir_load(array); struct hlsl_ir_resource_load *resource_load; if (index_type->type > HLSL_CLASS_VECTOR || index_type->dimx != dim_count) @@ -818,7 +775,7 @@ static bool add_array_load(struct hlsl_ctx *ctx, struct list *instrs, struct hls return false; load_params.format = expr_type->e.resource_format; - load_params.resource = object_load->src; + load_params.resource = array; load_params.coords = index; if (!(resource_load = hlsl_new_resource_load(ctx, &load_params, loc))) @@ -838,10 +795,7 @@ static bool add_array_load(struct hlsl_ctx *ctx, struct list *instrs, struct hls list_add_tail(instrs, &cast->node.entry); index = &cast->node; - if (expr_type->type == HLSL_CLASS_MATRIX) - return add_matrix_index(ctx, instrs, array, index, loc); - - if (expr_type->type != HLSL_CLASS_ARRAY && expr_type->type != HLSL_CLASS_VECTOR) + if (expr_type->type != HLSL_CLASS_ARRAY && expr_type->type != HLSL_CLASS_VECTOR && expr_type->type != HLSL_CLASS_MATRIX) { if (expr_type->type == HLSL_CLASS_SCALAR) hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_INDEX, "Scalar expressions cannot be array-indexed."); @@ -850,8 +804,9 @@ static bool add_array_load(struct hlsl_ctx *ctx, struct list *instrs, struct hls return false; } - if (!add_load_index(ctx, instrs, array, index, loc)) + if (!(return_index = hlsl_new_index(ctx, array, index, loc))) return false; + list_add_tail(instrs, &return_index->node.entry); return true; } @@ -1727,7 +1682,7 @@ static struct hlsl_ir_node *add_assignment(struct hlsl_ctx *ctx, struct list *in if (!(rhs = add_implicit_conversion(ctx, instrs, rhs, lhs_type, &rhs->loc))) return NULL; - while (lhs->type != HLSL_IR_LOAD && lhs->type != HLSL_IR_RESOURCE_LOAD) + while (lhs->type != HLSL_IR_LOAD && lhs->type != HLSL_IR_RESOURCE_LOAD && lhs->type != HLSL_IR_INDEX) { if (lhs->type == HLSL_IR_EXPR && hlsl_ir_expr(lhs)->op == HLSL_OP1_CAST) { @@ -1800,13 +1755,26 @@ static struct hlsl_ir_node *add_assignment(struct hlsl_ctx *ctx, struct list *in return NULL; list_add_tail(instrs, &store->node.entry); } + else if (lhs->type == HLSL_IR_INDEX && hlsl_index_is_crooked_matrix_indexing(hlsl_ir_index(lhs))) + { + hlsl_fixme(ctx, &lhs->loc, "Column-major matrix index store."); + return NULL; + } else { struct hlsl_ir_store *store; + struct hlsl_deref deref; - if (!(store = hlsl_new_store_index(ctx, &hlsl_ir_load(lhs)->src, NULL, rhs, writemask, &rhs->loc))) + if (!hlsl_init_deref_from_index_chain(ctx, &deref, lhs)) + return NULL; + + if (!(store = hlsl_new_store_index(ctx, &deref, NULL, rhs, writemask, &rhs->loc))) + { + hlsl_cleanup_deref(&deref); return NULL; + } list_add_tail(instrs, &store->node.entry); + hlsl_cleanup_deref(&deref); } /* Don't use the instruction itself as a source, as this makes structure @@ -3058,7 +3026,6 @@ static bool intrinsic_tex(struct hlsl_ctx *ctx, const struct parse_initializer * struct hlsl_resource_load_params load_params = {.type = HLSL_RESOURCE_SAMPLE}; const struct hlsl_type *sampler_type; struct hlsl_ir_resource_load *load; - struct hlsl_ir_load *sampler_load; struct hlsl_ir_node *coords; if (params->args_count != 2 && params->args_count != 4) @@ -3087,10 +3054,7 @@ static bool intrinsic_tex(struct hlsl_ctx *ctx, const struct parse_initializer * } else { - /* Only HLSL_IR_LOAD can return an object. */ - sampler_load = hlsl_ir_load(params->args[0]); - - load_params.resource = sampler_load->src; + load_params.resource = params->args[0]; } if (!(coords = add_implicit_conversion(ctx, params->instrs, params->args[1], @@ -3424,7 +3388,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl const char *name, const struct parse_initializer *params, const struct vkd3d_shader_location *loc) { const struct hlsl_type *object_type = object->data_type; - struct hlsl_ir_load *object_load; if (object_type->type != HLSL_CLASS_OBJECT || object_type->base_type != HLSL_TYPE_TEXTURE || object_type->sampler_dim == HLSL_SAMPLER_DIM_GENERIC) @@ -3438,9 +3401,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl return false; } - /* Only HLSL_IR_LOAD can return an object. */ - object_load = hlsl_ir_load(object); - if (!strcmp(name, "Load") && object_type->sampler_dim != HLSL_SAMPLER_DIM_CUBE && object_type->sampler_dim != HLSL_SAMPLER_DIM_CUBEARRAY) @@ -3484,7 +3444,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl return false; load_params.format = object_type->e.resource_format; - load_params.resource = object_load->src; + load_params.resource = object; if (!(load = hlsl_new_resource_load(ctx, &load_params, loc))) return false; @@ -3500,7 +3460,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl struct hlsl_resource_load_params load_params = {.type = HLSL_RESOURCE_SAMPLE}; const struct hlsl_type *sampler_type; struct hlsl_ir_resource_load *load; - struct hlsl_ir_load *sampler_load; if (params->args_count < 2 || params->args_count > 4 + !!offset_dim) { @@ -3523,9 +3482,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl return false; } - /* Only HLSL_IR_LOAD can return an object. */ - sampler_load = hlsl_ir_load(params->args[0]); - if (!(load_params.coords = add_implicit_conversion(ctx, instrs, params->args[1], hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc))) return false; @@ -3543,8 +3499,8 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl hlsl_fixme(ctx, loc, "Tiled resource status argument."); load_params.format = object_type->e.resource_format; - load_params.resource = object_load->src; - load_params.sampler = sampler_load->src; + load_params.resource = object; + load_params.sampler = params->args[0]; if (!(load = hlsl_new_resource_load(ctx, &load_params, loc))) return false; @@ -3564,7 +3520,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl struct hlsl_resource_load_params load_params = {0}; const struct hlsl_type *sampler_type; struct hlsl_ir_resource_load *load; - struct hlsl_ir_load *sampler_load; unsigned int read_channel; if (!strcmp(name, "GatherGreen")) @@ -3640,16 +3595,13 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl return false; } - /* Only HLSL_IR_LOAD can return an object. */ - sampler_load = hlsl_ir_load(params->args[0]); - if (!(load_params.coords = add_implicit_conversion(ctx, instrs, params->args[1], hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc))) return false; load_params.format = hlsl_get_vector_type(ctx, object_type->e.resource_format->base_type, 4); - load_params.resource = object_load->src; - load_params.sampler = sampler_load->src; + load_params.resource = object; + load_params.sampler = params->args[0]; if (!(load = hlsl_new_resource_load(ctx, &load_params, loc))) return false; @@ -3665,7 +3617,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl const unsigned int offset_dim = hlsl_offset_dim_count(object_type->sampler_dim); const struct hlsl_type *sampler_type; struct hlsl_ir_resource_load *load; - struct hlsl_ir_load *sampler_load; if (params->args_count < 3 || params->args_count > 4 + !!offset_dim) { @@ -3688,9 +3639,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl return false; } - /* Only HLSL_IR_LOAD can return an object. */ - sampler_load = hlsl_ir_load(params->args[0]); - if (!(load_params.coords = add_implicit_conversion(ctx, instrs, params->args[1], hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc))) load_params.coords = params->args[1]; @@ -3710,8 +3658,8 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl hlsl_fixme(ctx, loc, "Tiled resource status argument."); load_params.format = object_type->e.resource_format; - load_params.resource = object_load->src; - load_params.sampler = sampler_load->src; + load_params.resource = object; + load_params.sampler = params->args[0]; if (!(load = hlsl_new_resource_load(ctx, &load_params, loc))) return false; @@ -5296,7 +5244,7 @@ postfix_expr: } field_idx = field - type->e.record.fields; - if (!add_record_load(ctx, $1, node, field_idx, @2)) + if (!add_record_access(ctx, $1, node, field_idx, @2)) YYABORT; $$ = $1; } @@ -5325,7 +5273,7 @@ postfix_expr: list_move_tail($1, $3); vkd3d_free($3); - if (!add_array_load(ctx, $1, array, index, &@2)) + if (!add_array_access(ctx, $1, array, index, &@2)) { destroy_instr_list($1); YYABORT; diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 553aedef..ab2b4b02 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -731,6 +731,102 @@ static bool lower_calls(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void * return true; } +/* Single hlsl_ir_index instructions on column-major matrices cannot be turned into derefs, however, + * index chains that refer to a single component can be. This pass turns the former in the latter. */ +static bool lower_crooked_matrix_index_loads(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) +{ + struct hlsl_ir_index *index; + struct hlsl_deref var_deref; + struct hlsl_ir_index *copy; + struct hlsl_ir_load *load; + struct hlsl_ir_node *mat; + struct hlsl_ir_var *var; + unsigned int i; + + if (instr->type != HLSL_IR_INDEX) + return false; + index = hlsl_ir_index(instr); + + if (!hlsl_index_is_crooked_matrix_indexing(index)) + return false; + + mat = index->val.node; + assert(!hlsl_type_is_row_major(mat->data_type)); + + /* We will remove this instruction but we need to generate nodes that refer to it, + * so we create a copy. */ + if (!(copy = hlsl_new_index(ctx, index->val.node, index->idx.node, &instr->loc))) + return false; + list_add_before(&instr->entry, ©->node.entry); + + if (!(var = hlsl_new_synthetic_var(ctx, "index", instr->data_type, &instr->loc))) + return false; + hlsl_init_simple_deref_from_var(&var_deref, var); + + for (i = 0; i < mat->data_type->dimx; ++i) + { + struct hlsl_ir_index *value; + struct hlsl_ir_store *store; + struct hlsl_ir_constant *c; + struct hlsl_block block; + + if (!(c = hlsl_new_uint_constant(ctx, i, &instr->loc))) + return false; + list_add_before(&instr->entry, &c->node.entry); + + if (!(value = hlsl_new_index(ctx, ©->node, &c->node, &instr->loc))) + return false; + list_add_before(&instr->entry, &value->node.entry); + + if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, i, &value->node))) + return false; + list_move_before(&instr->entry, &block.instrs); + } + + if (!(load = hlsl_new_var_load(ctx, var, instr->loc))) + return false; + list_add_before(&instr->entry, &load->node.entry); + hlsl_replace_node(instr, &load->node); + + return true; +} + +/* hlsl_ir_index nodes are a parse-time construct used to represent array indexing and struct + * record access before knowing if they will be used in the lhs of an assignment --in which case + * they are lowered into a deref-- or as the load of an element within a larger value. + * For the latter case, this pass takes care of lowering hlsl_ir_indexes (and index chains) into + * individual hlsl_ir_loads. + * It is worth noting that the generated hlsl_ir_loads don't load from a copy of the variable loaded + * at the beggining of the index chain, but from the same variable instead, because it is assumed + * that the value of the variable won't change between the the hlsl_ir_load instruction and the + * hlsl_ir_index. */ +static bool lower_index_loads(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) +{ + struct hlsl_ir_index *index; + struct hlsl_ir_load *load; + struct hlsl_deref deref; + + if (instr->type != HLSL_IR_INDEX) + return false; + index = hlsl_ir_index(instr); + + if (hlsl_index_is_crooked_matrix_indexing(index)) + return false; + + if (!hlsl_init_deref_from_index_chain(ctx, &deref, instr)) + return false; + + if (!(load = hlsl_new_load_index(ctx, &deref, NULL, &instr->loc))) + { + hlsl_cleanup_deref(&deref); + return false; + } + list_add_before(&instr->entry, &load->node.entry); + hlsl_replace_node(instr, &load->node); + hlsl_cleanup_deref(&deref); + return true; +} + /* Lower casts from vec1 to vecN to swizzles. */ static bool lower_broadcasts(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) { @@ -3233,6 +3329,9 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry while (transform_ir(ctx, lower_calls, body, NULL)); + transform_ir(ctx, lower_crooked_matrix_index_loads, body, NULL); + transform_ir(ctx, lower_index_loads, body, NULL); + LIST_FOR_EACH_ENTRY(var, &ctx->globals->vars, struct hlsl_ir_var, scope_entry) { if (var->storage_modifiers & HLSL_STORAGE_UNIFORM) diff --git a/tests/hlsl-matrix-indexing.shader_test b/tests/hlsl-matrix-indexing.shader_test index 7ae1a91d..4ca0e871 100644 --- a/tests/hlsl-matrix-indexing.shader_test +++ b/tests/hlsl-matrix-indexing.shader_test @@ -78,7 +78,7 @@ draw quad probe all rgba (1.0, 5.0, 7.0, 12.0) -[pixel shader] +[pixel shader todo] float4 main() : SV_TARGET { float3x2 m = {1, 2, 3, 4, 5, 6}; @@ -89,7 +89,7 @@ float4 main() : SV_TARGET } [test] -draw quad +todo draw quad todo probe all rgba (30.0, 40.0, 5.0, 6.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124
From: Francisco Casas <fcasas(a)codeweavers.com> --- In v1 this call was replaced with a single hlsl_ir_index. But it can't be done this way now becase column-wise hlsl_ir_index were discarded. --- libs/vkd3d-shader/hlsl.y | 59 ++++++++-------------------------------- 1 file changed, 11 insertions(+), 48 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 4c64ef86..f86fa7cb 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -620,38 +620,6 @@ static struct hlsl_ir_jump *add_return(struct hlsl_ctx *ctx, struct list *instrs return jump; } -static struct hlsl_ir_load *add_load_index(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_instr, - struct hlsl_ir_node *idx, const struct vkd3d_shader_location *loc) -{ - const struct hlsl_deref *src; - struct hlsl_ir_load *load; - - if (var_instr->type == HLSL_IR_LOAD) - { - src = &hlsl_ir_load(var_instr)->src; - } - else - { - struct hlsl_ir_store *store; - struct hlsl_ir_var *var; - - if (!(var = hlsl_new_synthetic_var(ctx, "deref", var_instr->data_type, &var_instr->loc))) - return NULL; - - if (!(store = hlsl_new_simple_store(ctx, var, var_instr))) - return NULL; - list_add_tail(instrs, &store->node.entry); - - src = &store->lhs; - } - - if (!(load = hlsl_new_load_index(ctx, src, idx, loc))) - return NULL; - list_add_tail(instrs, &load->node.entry); - - return load; -} - static struct hlsl_ir_load *add_load_component(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *var_instr, unsigned int comp, const struct vkd3d_shader_location *loc) { @@ -1264,46 +1232,41 @@ static struct hlsl_ir_node *add_expr(struct hlsl_ctx *ctx, struct list *instrs, if (type->type == HLSL_CLASS_MATRIX) { - struct hlsl_type *vector_type; + struct hlsl_type *scalar_type; struct hlsl_deref var_deref; struct hlsl_ir_load *load; struct hlsl_ir_var *var; - vector_type = hlsl_get_vector_type(ctx, type->base_type, hlsl_type_minor_size(type)); + scalar_type = hlsl_get_scalar_type(ctx, type->base_type); if (!(var = hlsl_new_synthetic_var(ctx, "split_op", type, loc))) return NULL; hlsl_init_simple_deref_from_var(&var_deref, var); - for (i = 0; i < hlsl_type_major_size(type); ++i) + for (i = 0; i < type->dimy * type->dimx; ++i) { - struct hlsl_ir_node *value, *vector_operands[HLSL_MAX_OPERANDS] = { NULL }; + struct hlsl_ir_node *value, *cell_operands[HLSL_MAX_OPERANDS] = { NULL }; struct hlsl_ir_store *store; - struct hlsl_ir_constant *c; + struct hlsl_block block; unsigned int j; - if (!(c = hlsl_new_uint_constant(ctx, i, loc))) - return NULL; - list_add_tail(instrs, &c->node.entry); - for (j = 0; j < HLSL_MAX_OPERANDS; j++) { if (operands[j]) { - struct hlsl_ir_load *load; - - if (!(load = add_load_index(ctx, instrs, operands[j], &c->node, loc))) + if (!(load = add_load_component(ctx, instrs, operands[j], i, loc))) return NULL; - vector_operands[j] = &load->node; + + cell_operands[j] = &load->node; } } - if (!(value = add_expr(ctx, instrs, op, vector_operands, vector_type, loc))) + if (!(value = add_expr(ctx, instrs, op, cell_operands, scalar_type, loc))) return NULL; - if (!(store = hlsl_new_store_index(ctx, &var_deref, &c->node, value, 0, loc))) + if (!(store = hlsl_new_store_component(ctx, &block, &var_deref, i, value))) return NULL; - list_add_tail(instrs, &store->node.entry); + list_move_tail(instrs, &block.instrs); } if (!(load = hlsl_new_var_load(ctx, var, *loc))) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 24 ++++++++--------------- tests/hlsl-function.shader_test | 4 ++-- tests/hlsl-intrinsic-override.shader_test | 2 +- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index f86fa7cb..0ce42fcc 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -624,27 +624,19 @@ static struct hlsl_ir_load *add_load_component(struct hlsl_ctx *ctx, struct list unsigned int comp, const struct vkd3d_shader_location *loc) { const struct hlsl_deref *src; + struct hlsl_ir_store *store; struct hlsl_ir_load *load; struct hlsl_block block; + struct hlsl_ir_var *var; - if (var_instr->type == HLSL_IR_LOAD) - { - src = &hlsl_ir_load(var_instr)->src; - } - else - { - struct hlsl_ir_store *store; - struct hlsl_ir_var *var; - - if (!(var = hlsl_new_synthetic_var(ctx, "deref", var_instr->data_type, &var_instr->loc))) - return NULL; + if (!(var = hlsl_new_synthetic_var(ctx, "deref", var_instr->data_type, &var_instr->loc))) + return NULL; - if (!(store = hlsl_new_simple_store(ctx, var, var_instr))) - return NULL; - list_add_tail(instrs, &store->node.entry); + if (!(store = hlsl_new_simple_store(ctx, var, var_instr))) + return NULL; + list_add_tail(instrs, &store->node.entry); - src = &store->lhs; - } + src = &store->lhs; if (!(load = hlsl_new_load_component(ctx, &block, src, comp, loc))) return NULL; diff --git a/tests/hlsl-function.shader_test b/tests/hlsl-function.shader_test index a7cad6d7..4d4c2e2b 100644 --- a/tests/hlsl-function.shader_test +++ b/tests/hlsl-function.shader_test @@ -276,7 +276,7 @@ float4 main() : sv_target [test] draw quad -todo probe all rgba (2.0, 3.0, 6.0, 7.0) +probe all rgba (2.0, 3.0, 6.0, 7.0) [pixel shader] @@ -294,4 +294,4 @@ float4 main() : sv_target [test] draw quad -todo probe all rgba (2.0, 3.0, 6.0, 7.0) +probe all rgba (2.0, 3.0, 6.0, 7.0) diff --git a/tests/hlsl-intrinsic-override.shader_test b/tests/hlsl-intrinsic-override.shader_test index 13e1752d..55a23f21 100644 --- a/tests/hlsl-intrinsic-override.shader_test +++ b/tests/hlsl-intrinsic-override.shader_test @@ -12,7 +12,7 @@ float4 main() : sv_target [test] draw quad -todo probe all rgba (0.3, 0.3, 0.4, 0.6) +probe all rgba (0.3, 0.3, 0.4, 0.6) [pixel shader] -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 39 ++++++++++++++++++++++++-- tests/hlsl-matrix-indexing.shader_test | 6 ++-- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 0ce42fcc..6ecd1905 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1712,8 +1712,43 @@ static struct hlsl_ir_node *add_assignment(struct hlsl_ctx *ctx, struct list *in } else if (lhs->type == HLSL_IR_INDEX && hlsl_index_is_crooked_matrix_indexing(hlsl_ir_index(lhs))) { - hlsl_fixme(ctx, &lhs->loc, "Column-major matrix index store."); - return NULL; + struct hlsl_ir_index *row = hlsl_ir_index(lhs); + struct hlsl_ir_node *mat = row->val.node; + unsigned int i, k = 0; + + for (i = 0; i < mat->data_type->dimx; ++i) + { + struct hlsl_ir_store *store; + struct hlsl_ir_constant *c; + struct hlsl_ir_index *cell; + struct hlsl_ir_load *load; + struct hlsl_deref deref; + + if (!(writemask & (1 << i))) + continue; + + if (!(c = hlsl_new_uint_constant(ctx, i, &lhs->loc))) + return NULL; + list_add_tail(instrs, &c->node.entry); + + if (!(cell = hlsl_new_index(ctx, &row->node, &c->node, &lhs->loc))) + return NULL; + list_add_tail(instrs, &cell->node.entry); + + if (!(load = add_load_component(ctx, instrs, rhs, k++, &rhs->loc))) + return NULL; + + if (!hlsl_init_deref_from_index_chain(ctx, &deref, &cell->node)) + return NULL; + + if (!(store = hlsl_new_store_index(ctx, &deref, NULL, &load->node, 0, &rhs->loc))) + { + hlsl_cleanup_deref(&deref); + return NULL; + } + list_add_tail(instrs, &store->node.entry); + hlsl_cleanup_deref(&deref); + } } else { diff --git a/tests/hlsl-matrix-indexing.shader_test b/tests/hlsl-matrix-indexing.shader_test index 4ca0e871..1dc1ede0 100644 --- a/tests/hlsl-matrix-indexing.shader_test +++ b/tests/hlsl-matrix-indexing.shader_test @@ -78,7 +78,7 @@ draw quad probe all rgba (1.0, 5.0, 7.0, 12.0) -[pixel shader todo] +[pixel shader] float4 main() : SV_TARGET { float3x2 m = {1, 2, 3, 4, 5, 6}; @@ -89,8 +89,8 @@ float4 main() : SV_TARGET } [test] -todo draw quad -todo probe all rgba (30.0, 40.0, 5.0, 6.0) +draw quad +probe all rgba (30.0, 40.0, 5.0, 6.0) [pixel shader] -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124
Removed `hlsl_ir_index.is_colum.is_column_indexing`. I think it looks good. In 5/7 the call to `add_load_index()` cannot be replaced with a single `hlsl_new_index()` now, but component store/loads are used instead. The new scheme required separating the chunk a code from the `lower_index_loads` in the new `lower_crooked_matrix_index_loads` pass. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27228
Perhaps in the future we might even consider using the lower_index_loads pass together with the constant folding passes on the rules for every `statement` and other non-terminal symbols. I don't think I see the idea here?
I was just thinking that, since at some point we will need to do constant-folding at parse time (for instance just at the end of every `statement:` rule), for folding array indexes, array sizes, and initialization expressions, it may make sense to treat the `lower_index_loads` pass in the same way, because we may also need the passes to split loads there. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27229
I was just thinking that, since at some point we will need to do constant-folding at parse time (for instance just at the end of every `statement:` rule), for folding array indexes, array sizes, and initialization expressions, it may make sense to treat the `lower_index_loads` pass in the same way, because we may also need the passes to split loads there.
We'll definitely need multiple passes to resolve constant expressions, and I expect that lower_index_loads() may be one of them. It probably makes most sense to add those as needed. Performing those passes at the end of every statement rule seems questionable, and I think won't work anyway (since we need to lower expressions, not statements). Probably it should just be done when we need to, in evaluate_constant_expression() or whatever it's called now. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27371
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
HLSL_IR_RESOURCE_STORE, HLSL_IR_STORE, HLSL_IR_SWIZZLE, + HLSL_IR_INDEX,
Not that it's a big deal, but the rest of these are alphabetized, here and in a lot of other places. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27456
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.c:
return swizzle; }
+bool hlsl_index_is_crooked_matrix_indexing(struct hlsl_ir_index *index) +{ + struct hlsl_type *type = index->val.node->data_type; + + return type->type == HLSL_CLASS_MATRIX && !hlsl_type_is_row_major(type); +}
So I think the dead code in 3/7 is fine, even for upstream, because it's simple and clear what it's used for. hlsl_index_is_crooked_matrix_indexing(), on the other hand, isn't used in this patch (anymore), and is less clear, so I'd be inclined to defer it until it's used. Wrt naming, perhaps invert the meaning and say "contiguous" rather than "crooked"? (And "matrix_indexing" is probably redundant.) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27458
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.c:
return swizzle; }
+bool hlsl_index_is_crooked_matrix_indexing(struct hlsl_ir_index *index) +{ + struct hlsl_type *type = index->val.node->data_type; + + return type->type == HLSL_CLASS_MATRIX && !hlsl_type_is_row_major(type); +} + +struct hlsl_ir_index *hlsl_new_index(struct hlsl_ctx *ctx, struct hlsl_ir_node *val, + struct hlsl_ir_node *idx, const struct vkd3d_shader_location *loc)
For new code I think we want to return hlsl_ir_node here, unless there's some reason we can't quite do that? (The basic reason, which I might have gone into before, is that this firstly simplifies code a bit, by turning e.g. "&index->node" into "index" in a lot of places, and secondly, I have plans in the works that we can then return nodes of type HLSL_IR_ERROR, potentially negating the need to check for allocation failure.) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27459
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.c:
return true; }
+bool hlsl_init_deref_from_index_chain(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_node *chain) +{ + struct hlsl_ir_index *index; + struct hlsl_ir_load *load; + unsigned int chain_len, i; + struct hlsl_ir_node *ptr; + + deref->path = NULL; + deref->path_len = 0; + deref->offset.node = NULL; + + if (!chain) + return true;
I'd be mildly inclined to punt this to the caller, it seems less confusing to someone reading this function without context. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27460
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.c:
+ { + unsigned int p = deref->path_len - 1 - i; + + index = hlsl_ir_index(ptr); + if (!hlsl_index_is_crooked_matrix_indexing(index)) + { + hlsl_src_from_node(&deref->path[p], index->idx.node); + } + else + { + hlsl_src_from_node(&deref->path[p], deref->path[p + 1].node); + hlsl_src_remove(&deref->path[p + 1]); + hlsl_src_from_node(&deref->path[p + 1], index->idx.node); + } + ptr = index->val.node; + } Maybe it'd be clearer to handle the noncontiguous matrix case by checking if this iteration is a scalar whose parent is a noncontiguous matrix dereference?
Also, if the previous loop is "while (ptr->type == HLSL_IR_INDEX)", I'd kind of like to make this loop the same, just to make it a bit more idiomatic / clearer that it's the same iteration. (And then one can assert that chain_len == load->src.path_len or something.) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27461
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
+ + if (instr->type != HLSL_IR_INDEX) + return false; + index = hlsl_ir_index(instr); + + if (!hlsl_index_is_crooked_matrix_indexing(index)) + return false; + + mat = index->val.node; + assert(!hlsl_type_is_row_major(mat->data_type)); + + /* We will remove this instruction but we need to generate nodes that refer to it, + * so we create a copy. */ + if (!(copy = hlsl_new_index(ctx, index->val.node, index->idx.node, &instr->loc))) + return false; + list_add_before(&instr->entry, ©->node.entry); Oh, hlsl_replace_node() deletes the old instruction. Hmm, maybe we could introduce a variant that doesn't do that instead?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27462
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
+ + if (!(load = hlsl_new_var_load(ctx, var, instr->loc))) + return false; + list_add_before(&instr->entry, &load->node.entry); + hlsl_replace_node(instr, &load->node); + + return true; +} + +/* hlsl_ir_index nodes are a parse-time construct used to represent array indexing and struct + * record access before knowing if they will be used in the lhs of an assignment --in which case + * they are lowered into a deref-- or as the load of an element within a larger value. + * For the latter case, this pass takes care of lowering hlsl_ir_indexes (and index chains) into + * individual hlsl_ir_loads. + * It is worth noting that the generated hlsl_ir_loads don't load from a copy of the variable loaded + * at the beggining of the index chain, but from the same variable instead, because it is assumed Spelling error, "beginning".
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27463
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
+ return false; + list_add_before(&instr->entry, &load->node.entry); + hlsl_replace_node(instr, &load->node); + + return true; +} + +/* hlsl_ir_index nodes are a parse-time construct used to represent array indexing and struct + * record access before knowing if they will be used in the lhs of an assignment --in which case + * they are lowered into a deref-- or as the load of an element within a larger value. + * For the latter case, this pass takes care of lowering hlsl_ir_indexes (and index chains) into + * individual hlsl_ir_loads. + * It is worth noting that the generated hlsl_ir_loads don't load from a copy of the variable loaded + * at the beggining of the index chain, but from the same variable instead, because it is assumed + * that the value of the variable won't change between the the hlsl_ir_load instruction and the + * hlsl_ir_index. */ I don't like this assumption. Isn't this exactly the kind of thing we're trying to avoid?
Then again, the obvious way to actually trigger this is insanity like float4 main() : sv_target { float x[3] = {0, 2, 3}; return x[x[1] = 1]; } which actually *does* yield 1. But it still makes me nervous. I'm not sure if there are other ways that this might do the wrong thing. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27464
Nitpick on the title of #7: should probably be "support column-major matrix indexing" specifically. Otherwise the series looks pretty great [modulo my other nitpicks]. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27465
So I think the dead code in 3/7 is fine, even for upstream, because it's simple and clear what it's used for. hlsl_index_is_crooked_matrix_indexing(), on the other hand, isn't used in this patch (anymore), and is less clear, so I'd be inclined to defer it until it's used.
Okay.
Wrt naming, perhaps invert the meaning and say "contiguous" rather than "crooked"? (And "matrix_indexing" is probably redundant.)
I changed the name to "hlsl_index_is_noncontiguous". I think it is better not to invert the naming (and the return value) because this is a rather exceptional case, and it is often the one that deserves special treatment. Also, in part 2 of the series, I want to discuss a patch for also using `hlsl_ir_index` for resource accesses, which introduces a similar function, for which the inverted naming would be inappropriate: ```c bool hlsl_index_is_resource_access(struct hlsl_ir_index *index); ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27671
On Wed Mar 22 21:40:35 2023 +0000, Zebediah Figura wrote:
I'd be mildly inclined to punt this to the caller, it seems less confusing to someone reading this function without context. Good point. I was only using that for initializing the `hlsl_ir_resource_load.sampler` deref.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27672
On Tue Mar 21 22:24:19 2023 +0000, Zebediah Figura wrote:
I don't like this assumption. Isn't this exactly the kind of thing we're trying to avoid? Then again, the obvious way to actually trigger this is insanity like float4 main() : sv_target { float x[3] = {0, 2, 3}; return x[x[1] = 1]; } which actually *does* yield 1. But it still makes me nervous. I'm not sure if there are other ways that this might do the wrong thing. I wanted to avoid copying the load into a synthetic variable and then loading from it, since the generated IR before dce is getting increasingly bloated, which may make debugging difficult. But cases such as the one you mention didn't occur to me, good catch!
Adding a function call to your example actually makes the current implementation fail (retrieving 1.0 instead of 11.0): ```hlsl uint4 func(uint t) { return uint4(t + 0, t + 1, t + 2, t + 3); } float4 main() : sv_target { return func(10)[func(0).y]; } ``` To solve this I had to bring a patch from the future (to support indexing non-hlsl_ir_load expressions) and change it a little. I also brought the related tests. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27675
On Thu Mar 23 21:31:37 2023 +0000, Zebediah Figura wrote:
For new code I think we want to return hlsl_ir_node here, unless there's some reason we can't quite do that? (The basic reason, which I might have gone into before, is that this firstly simplifies code a bit, by turning e.g. "&index->node" into "index" in a lot of places, and secondly, I have plans in the works that we can then return nodes of type HLSL_IR_ERROR, potentially negating the need to check for allocation failure.) Okay, I think it helps :)
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27676
Maybe it'd be clearer to handle the noncontiguous matrix case by checking if this iteration is a scalar whose parent is a noncontiguous matrix dereference?
Since the loop goes from the component to the variable, rather than the other way around, that would require paying attention to the next iteration, rather than the previous one, and advance `ptr` two steps in that case. I don't think it improves simplicity.
Also, if the previous loop is "while (ptr->type == HLSL_IR_INDEX)", I'd kind of like to make this loop the same, just to make it a bit more idiomatic / clearer that it's the same iteration. (And then one can assert that chain_len == load->src.path_len or something.)
Makes sense, I changed it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27673
On Tue Mar 21 22:24:19 2023 +0000, Zebediah Figura wrote:
Oh, hlsl_replace_node() deletes the old instruction. Hmm, maybe we could introduce a variant that doesn't do that instead? In this case it is still better to create the copy though, because otherwise we would need a way to specify which uses must be transferred to the replacement and which ones must be kept pointing to the original instruction.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/124#note_27674
participants (3)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura (＠zfigura)