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.
From: Francisco Casas fcasas@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)
From: Francisco Casas fcasas@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)
From: Francisco Casas fcasas@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 follow patch. I organized it this way to somewhat ease the review. --- libs/vkd3d-shader/hlsl.c | 87 ++++++++++++++++++++++++++++++++ libs/vkd3d-shader/hlsl.h | 19 +++++++ libs/vkd3d-shader/hlsl.y | 1 + libs/vkd3d-shader/hlsl_codegen.c | 9 ++++ 4 files changed, 116 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 24822e97..f0fc3d9e 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1274,6 +1274,45 @@ 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) == index->is_column_indexing); +} + +struct hlsl_ir_index *hlsl_new_index(struct hlsl_ctx *ctx, struct hlsl_ir_node *val, + struct hlsl_ir_node *idx, bool is_column_indexing, 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) + { + if (is_column_indexing) + type = hlsl_get_vector_type(ctx, type->base_type, type->dimy); + else + 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); + index->is_column_indexing = is_column_indexing; + 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 +1572,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->is_column_indexing, &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 +1620,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 +1999,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 +2371,24 @@ 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); + if (index->val.node->data_type->type == HLSL_CLASS_MATRIX) + { + if (index->is_column_indexing) + vkd3d_string_buffer_printf(buffer, "[col:"); + else + vkd3d_string_buffer_printf(buffer, "[row:"); + } + else + { + 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 +2443,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 +2594,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 +2650,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..1900bcce 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,14 @@ struct hlsl_ir_swizzle DWORD swizzle; };
+struct hlsl_ir_index +{ + struct hlsl_ir_node node; + struct hlsl_src val, idx; + + bool is_column_indexing; +}; + /* Reference to a variable, or a part of it (e.g. a vector within a matrix within a struct). */ struct hlsl_deref { @@ -848,6 +857,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 +1063,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, bool is_column_indexing, 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;
From: Francisco Casas fcasas@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 | 62 +++++++++++- libs/vkd3d-shader/hlsl.h | 3 +- libs/vkd3d-shader/hlsl.y | 128 ++++++++----------------- libs/vkd3d-shader/hlsl_codegen.c | 85 ++++++++++++++++ tests/hlsl-matrix-indexing.shader_test | 4 +- 5 files changed, 187 insertions(+), 95 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index f0fc3d9e..e3d306cb 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -435,6 +435,61 @@ 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_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; + + /* Find the length of the index chain */ + chain_len = 0; + ptr = chain; + while (ptr->type == HLSL_IR_INDEX) + { + struct hlsl_ir_index *index = hlsl_ir_index(ptr); + + assert(!hlsl_index_is_crooked_matrix_indexing(index)); + 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 + * 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) + { + struct hlsl_ir_index *index = hlsl_ir_index(ptr); + unsigned int p = deref->path_len - 1 - i; + + hlsl_src_from_node(&deref->path[p], 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 +1280,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 1900bcce..8a586c5f 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -787,7 +787,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; };
@@ -999,6 +999,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..6313b10e 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, false, &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, false, 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..38db824f 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -731,6 +731,89 @@ static bool lower_calls(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void * 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; + + if (instr->type != HLSL_IR_INDEX) + return false; + + index = hlsl_ir_index(instr); + + if (hlsl_index_is_crooked_matrix_indexing(index)) + { + struct hlsl_ir_node *mat = index->val.node, *idx = index->idx.node; + struct hlsl_deref var_deref; + struct hlsl_ir_load *load; + struct hlsl_ir_var *var; + unsigned int i; + + assert(!hlsl_type_is_row_major(mat->data_type)); + + 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 *column, *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 (!(column = hlsl_new_index(ctx, mat, &c->node, true, &instr->loc))) + return false; + list_add_before(&instr->entry, &column->node.entry); + + if (!(value = hlsl_new_index(ctx, &column->node, idx, false, &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; + } + else + { + struct hlsl_ir_load *load; + struct hlsl_deref deref; + + 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 +3316,8 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry
while (transform_ir(ctx, lower_calls, body, NULL));
+ while (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)
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl.y | 40 +++++----------------------------------- 1 file changed, 5 insertions(+), 35 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 6313b10e..ceaae22a 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) { @@ -1290,11 +1258,13 @@ static struct hlsl_ir_node *add_expr(struct hlsl_ctx *ctx, struct list *instrs, { if (operands[j]) { - struct hlsl_ir_load *load; + struct hlsl_ir_index *index;
- if (!(load = add_load_index(ctx, instrs, operands[j], &c->node, loc))) + if (!(index = hlsl_new_index(ctx, operands[j], &c->node, !hlsl_type_is_row_major(type), loc))) return NULL; - vector_operands[j] = &load->node; + list_add_tail(instrs, &index->node.entry); + + vector_operands[j] = &index->node; } }
From: Francisco Casas fcasas@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 ceaae22a..a7ffe951 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]
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl.y | 38 ++++++++++++++++++++++++-- tests/hlsl-matrix-indexing.shader_test | 6 ++-- 2 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index a7ffe951..719f4b35 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1719,8 +1719,42 @@ 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_node *mat = hlsl_ir_index(lhs)->val.node, *idx = hlsl_ir_index(lhs)->idx.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_load *load; + struct hlsl_ir_index *col; + 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 (!(col = hlsl_new_index(ctx, mat, &c->node, true, &lhs->loc))) + return NULL; + list_add_tail(instrs, &col->node.entry); + + if (!(load = add_load_component(ctx, instrs, rhs, k++, &rhs->loc))) + return NULL; + + if (!hlsl_init_deref_from_index_chain(ctx, &deref, &col->node)) + return NULL; + + if (!(store = hlsl_new_store_index(ctx, &deref, idx, &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]
I haven't done a full review yet, but in general this looks about exactly like what I was expecting, so very nice. Two major questions:
* Patch 6/7 ("vkd3d-shader/hlsl: Always load from a synthetic copy in add_load_component().") is the slightly iffy one to me. It makes me nervous that we're generating LOAD instructions during parsing. On the other hand it's not hard to see that it never gets called in a way that can put those loads on the LHS, so I don't hate it. The alternative of course would be to create something like hlsl_new_index_component() and then rely on lowering, but that would require extra code. Although actually it seems that this is the only caller of hlsl_new_load_component() so maybe that's worthwhile anyway? I dunno...
* Instead of worrying about matrix majority for indices, can we just say that HLSL_IR_INDEX ignores majority and always returns columns (or is it rows?), and rely on lower_index_loads() to split those indexes into multiple loads if necessary? add_expr() in that case would just use dimy (or is it dimx?) instead of hlsl_type_major_size().
- Patch 6/7 ("vkd3d-shader/hlsl: Always load from a synthetic copy in add_load_component().") is the slightly iffy one to me. It makes me nervous that we're generating LOAD instructions during parsing. On the other hand it's not hard to see that it never gets called in a way that can put those loads on the LHS, so I don't hate it.
I don't think that generating loads during parsing is inherently bad. In my opinion what is bad is to use the loads' derefs for generating the stores' lhs derefs, which is what we are getting rid of here.
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.
add_load_component() should be only used to load and not to store a particular component, for the latter task there it is hlsl_new_store_component(), which doesn't generate loads.
- Instead of worrying about matrix majority for indices, can we just say that HLSL_IR_INDEX ignores majority and always returns columns (or is it rows?), and rely on lower_index_loads() to split those indexes into multiple loads if necessary?
In HLSL, indexing a matrix always accesses a row. But by necessity our deref paths expect to access rows first for row-major matrices and columns first for column major matrices.
In this implementation, hlsl_init_deref_from_index_chain() expects the indexes of row-major matrices to be `is_column_indexing = false` and the indexes of column-major matrices to be `is_column_indexing = true`.
When parsing array indexings, the hlsl_ir_indexes are always created with `is_column_indexing = false`, and lower_index_loads() takes care of transforming one row load into several column loads for column-major matrices, before creating the final loads.
However, this is not enough for storing to a matrix row, where the index chain must be used immediately to generate the deref. For cases such as these, index instructions are created with `is_column_indexing = true` for hlsl_init_deref_from_index_chain() to work (which is implemented in 7/7).
These are also used to simplify the implementation of lower_index_loads() and add_expr() which otherwise would have to load and store one component at the time, that's if we insert the logic for swapping the last two nodes in the deref path in hlsl_init_deref_from_index_chain() for column-major matrices.
I can spend some time checking how an implementation without the `is_column_indexing` field would work. It would remove the argument in hlsl_new_index but it may be at the expense of less simplicity in these hlsl_init_deref_from_index_chain() callers.
add_expr() in that case would just use dimy (or is it dimx?) instead of hlsl_type_major_size().
I don't think this will simplify much here though, since hlsl_new_store_index() is also called and it expects the index argument according to the matrix majority.
- Patch 6/7 ("vkd3d-shader/hlsl: Always load from a synthetic copy in add_load_component().") is the slightly iffy one to me. It makes me nervous that we're generating LOAD instructions during parsing. On the other hand it's not hard to see that it never gets called in a way that can put those loads on the LHS, so I don't hate it.
I don't think that generating loads during parsing is inherently bad. In my opinion what is bad is to use the loads' derefs for generating the stores' lhs derefs, which is what we are getting rid of here.
Agreed on all counts, it's just a matter of being careful that we never *do* get there.
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?
- Instead of worrying about matrix majority for indices, can we just say that HLSL_IR_INDEX ignores majority and always returns columns (or is it rows?), and rely on lower_index_loads() to split those indexes into multiple loads if necessary?
In HLSL, indexing a matrix always accesses a row. But by necessity our deref paths expect to access rows first for row-major matrices and columns first for column major matrices.
In this implementation, hlsl_init_deref_from_index_chain() expects the indexes of row-major matrices to be `is_column_indexing = false` and the indexes of column-major matrices to be `is_column_indexing = true`.
When parsing array indexings, the hlsl_ir_indexes are always created with `is_column_indexing = false`, and lower_index_loads() takes care of transforming one row load into several column loads for column-major matrices, before creating the final loads.
However, this is not enough for storing to a matrix row, where the index chain must be used immediately to generate the deref. For cases such as these, index instructions are created with `is_column_indexing = true` for hlsl_init_deref_from_index_chain() to work (which is implemented in 7/7).
These are also used to simplify the implementation of lower_index_loads() and add_expr() which otherwise would have to load and store one component at the time, that's if we insert the logic for swapping the last two nodes in the deref path in hlsl_init_deref_from_index_chain() for column-major matrices.
I can spend some time checking how an implementation without the `is_column_indexing` field would work. It would remove the argument in hlsl_new_index but it may be at the expense of less simplicity in these hlsl_init_deref_from_index_chain() callers.
Right, we'd have to split it up on the lhs too. And yes, it means that hlsl_init_deref_from_index_chain() can't exactly exist as it is. It still seems like the right approach to me, but I'm willing to be proven wrong...
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Zebediah Figura (＠zfigura)