July 2022 - Wine-Devel - list.winehq.org

Re: [PATCH vkd3d v3 16/17] vkd3d-shader/hlsl: Replace register offsets with index paths in input/output copies.

by Matteo Bruni

On Wed, Jul 20, 2022 at 3:24 PM Francisco Casas <fcasas(a)codeweavers.com> wrote: > > Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> > --- > > v3: > * No changes. > > The recursive structure of prepend_input_var_copy() and > append_output_var_copy() could be preserved creating additional > loads to complete the paths. Otherwise we would be requiring > passing whole paths as arguments. > > These additional loads should be handled by DCE. > > Still, matrix vectors are copied iteratively instead of recursively now, > to avoid the boilerplate of creating new loads in this last step. > > Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> > --- > libs/vkd3d-shader/hlsl_codegen.c | 188 +++++++++++++++++++------------ > 1 file changed, 116 insertions(+), 72 deletions(-) > > diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c > index 6437006b..7a245007 100644 > --- a/libs/vkd3d-shader/hlsl_codegen.c > +++ b/libs/vkd3d-shader/hlsl_codegen.c > @@ -239,59 +239,75 @@ static struct hlsl_ir_var *add_semantic_var(struct hlsl_ctx *ctx, struct hlsl_ir > return ext_var; > } > > -static void prepend_input_copy(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_var *var, > - struct hlsl_type *type, unsigned int field_offset, unsigned int modifiers, const struct hlsl_semantic *semantic) > +static void prepend_input_copy(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_load *lhs, > + struct hlsl_type *type, unsigned int modifiers, const struct hlsl_semantic *semantic) I haven't put a lot of thought into this, but it seems to me we could pass a deref instead of a redundant load and be mostly set. Mostly because we then need to add the instructions to the instruction list in some other way (e.g. by passing an explicit list).

July 28, 2022

Re: [PATCH vkd3d v3 09/17] vkd3d-shader/hlsl: Replace register offsets with index paths in load initializations.

by Matteo Bruni

On Wed, Jul 20, 2022 at 3:23 PM Francisco Casas <fcasas(a)codeweavers.com> wrote: > > The transform_deref_paths_into_offsets pass turns these index paths back > into register offsets. > > Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> > --- > > The idea is that we can move the transform_deref_paths_into_offsets() > pass forward as we translate more passes to work with index paths. > This, until register offsets can be totally removed, after we implement > the SMxIRs and their translations. > > The aim is to have 3 ways of initializing load/store nodes when using index > paths: > > * One that initializes the node from a another's node deref and and > optional index to be appended to that deref's path. > * One that initializes the node from a deref and the index > of a single basic component within it. This one also generates constant > nodes for the required path, so it also initializes an instruction block > whose instructions must be inserted in the instruction list. > * One that initializes the node directly for a whole variable. These functions > are already present: hlsl_new_var_load() and hlsl_new_simple_store(). > > The signatures of these functions are to be placed nearby in hlsl.h as > they are introduced in the following patches. > > It is worth noting that the use of index paths allows to remove the data type > argument when initializing store/loads because it can now be deducted from the > variable and the hlsl_deref. > > Applying an index over a matrix derefence retrieves a vector. If the matrix > is row_major, this corresponds to a row, otherwise, it corresponds to a > column. So, the code should take matrix majority into account, at least until > the split_matrix_copies pass. > > The first index in a path after a loading a struct should be an > hlsl_ir_constant, since the field that's being addressed is always > known at parse-time. > > hlsl_init_simple_deref_from_var() can be used to initialize a deref that can > be passed by reference to the load and store initialization functions. > This value shall not be modified after being created and does not > require to call hlsl_cleanup_deref(). > The deref obtained with this function, can also be passed be passed as prefix > to deref_from_component_index(). > > --- > > v3: > * Replaced compute_component_path() with deref_from_component_index(). > * Wrote implementation of init_deref() and get_type_from_deref() further > up in the file. > * Made hlsl_new_load_component() use deref_from_component_index() > instead of the removed compute_component_path(). > * Rewrote hlsl.c function comments in present and active voice. > * Renamed > typep -> type_ptr > indexp -> index_ptr > in subtype_index_from_component_index(). > * Added space before '?' in ternary operators. > > Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> > --- > libs/vkd3d-shader/hlsl.c | 298 +++++++++++++++++++++++++++++-- > libs/vkd3d-shader/hlsl.h | 35 +++- > libs/vkd3d-shader/hlsl.y | 102 ++++++----- > libs/vkd3d-shader/hlsl_codegen.c | 44 +++++ > 4 files changed, 410 insertions(+), 69 deletions(-) > > diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c > index 66acce23..535433ee 100644 > --- a/libs/vkd3d-shader/hlsl.c > +++ b/libs/vkd3d-shader/hlsl.c > @@ -330,6 +330,176 @@ unsigned int hlsl_compute_component_offset(struct hlsl_ctx *ctx, struct hlsl_typ > return 0; > } > > +static bool type_is_single_component(const struct hlsl_type *type) > +{ > + return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_OBJECT; > +} > + > +/* Given a type and a component index, this function returns the next path index required to reach > + * the component within the type. > + * It sets *type to the subtype within the original type that contains the component. > + * It sets *index to the index of the component within *type. */ Now *type_ptr and *index_ptr respectively, right? > +static unsigned int subtype_index_from_component_index(struct hlsl_ctx *ctx, > + struct hlsl_type **type_ptr, unsigned int *index_ptr) After rereading the patch a number of times, I think the prototype is okay (not sure about the name, it sounds like a "get" but it does change the two parameters, not that I have better suggestions) but that comment above needs some clarification. What this function does is to move one step through the "path", or component index I guess, returning the "index" to be taken for the outer data type and updating type_ptr and index_ptr (which are two in/out parameters btw, their types do make it somewhat expected but I wouldn't say it's super obvious at a first glance) with the next inner type and index. Which I guess is what the comment is trying to convey, but dunno, it really didn't work for me. It might be enough to tweak it with more details (e.g. next -> outer?) or maybe it deserves some more substantial rewriting. > +{ > + struct hlsl_type *type = *type_ptr; > + unsigned int index = *index_ptr; > + > + assert(!type_is_single_component(type)); > + assert(index < hlsl_type_component_count(type)); > + > + switch (type->type) > + { > + case HLSL_CLASS_VECTOR: > + assert(index < type->dimx); > + *type_ptr = hlsl_get_scalar_type(ctx, type->base_type); > + *index_ptr = 0; > + return index; > + > + case HLSL_CLASS_MATRIX: > + { > + unsigned int y = index / type->dimx, x = index % type->dimx; > + bool row_major = hlsl_type_is_row_major(type); > + > + assert(index < type->dimx * type->dimy); > + *type_ptr = hlsl_get_vector_type(ctx, type->base_type, row_major ? type->dimx : type->dimy); > + *index_ptr = row_major ? x : y; > + return row_major ? y : x; > + } > + > + case HLSL_CLASS_ARRAY: > + { > + unsigned int elem_comp_count = hlsl_type_component_count(type->e.array.type); > + unsigned int array_index; > + > + *type_ptr = type->e.array.type; > + *index_ptr = index % elem_comp_count; > + array_index = index / elem_comp_count; > + assert(array_index < type->e.array.elements_count); > + return array_index; > + } > + > + case HLSL_CLASS_STRUCT: > + { > + struct hlsl_struct_field *field; > + unsigned int field_comp_count, i; > + > + for (i = 0; i < type->e.record.field_count; ++i) > + { > + field = &type->e.record.fields[i]; > + field_comp_count = hlsl_type_component_count(field->type); > + if (index < field_comp_count) > + { > + *type_ptr = field->type; > + *index_ptr = index; > + return i; > + } > + index -= field_comp_count; > + } > + assert(0); > + return 0; > + } > + > + default: > + assert(0); > + return 0; > + } > +} > + > +struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type, > + unsigned int index) > +{ > + while (!type_is_single_component(type)) > + subtype_index_from_component_index(ctx, &type, &index); > + > + return type; > +} > + > +static bool init_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_var *var, > + unsigned int path_len) > +{ > + deref->var = var; > + deref->path_len = path_len; > + deref->offset.node = NULL; > + > + if (path_len == 0) > + { > + deref->path = NULL; > + return true; > + } > + > + if (!(deref->path = hlsl_alloc(ctx, sizeof(*deref->path) * deref->path_len))) > + { > + deref->var = NULL; > + deref->path_len = 0; > + return false; > + } > + > + return true; > +} > + > +static struct hlsl_type *get_type_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref) > +{ > + struct hlsl_type *type; > + unsigned int i; > + > + assert(deref); > + assert(!deref->offset.node); > + > + type = deref->var->data_type; > + for (i = 0; i < deref->path_len; ++i) > + type = hlsl_get_type_from_path_index(ctx, type, deref->path[i].node); > + return type; > +} > + > +/* Initializes a deref from another deref (prefix) and a component index. */ > +static bool deref_from_component_index(struct hlsl_ctx *ctx, struct hlsl_block *block, > + struct hlsl_deref *deref, const struct hlsl_deref *prefix, unsigned int index, > + const struct vkd3d_shader_location *loc) It might come as a shock but I don't like this function's name. Originally these x_from_y() kind of functions were always "conversion" functions, i.e. take object y and return something from it as type x. Which I guess is technically what this function is, but I don't feel like that is the best way to represent it. For one, this function effectively has 2 out parameters (block and deref), in that it also generates instructions and adds them to the block list. I guess it's not any different from a few other functions you're introducing in the patchset but, as it is, the comment feels somewhat partial. I would mention "block" as another byproduct of the function in the comment at the top. In regard to the name, maybe just adding a new_ (or create_? generate_? init_?) in front would be enough. I don't have great suggestions, as usual. Somewhat minor, and probably more controversial, but maybe still worth mentioning: instead of returning a bool, the function could return a pointer to the deref. It should return NULL in the codepaths currently returning "false", to maintain the current semantics (which are fine). While it's unlikely that the return value is going to be super useful, making this function resemble the other similar ones introduced in this patch and previously has value in my mind. Also, of course, that doesn't mean you can drop the "deref" parameter. > +{ > + unsigned int path_len, path_index, deref_path_len, i; > + struct hlsl_type *path_type; > + struct hlsl_ir_constant *c; > + > + list_init(&block->instrs); > + > + path_len = 0; > + path_type = get_type_from_deref(ctx, prefix); > + path_index = index; > + while (!type_is_single_component(path_type)) > + { > + subtype_index_from_component_index(ctx, &path_type, &path_index); > + ++path_len; > + } > + > + if (!init_deref(ctx, deref, prefix->var, prefix->path_len + path_len)) > + return false; > + > + deref_path_len = 0; > + for (i = 0; i < prefix->path_len; ++i) > + hlsl_src_from_node(&deref->path[deref_path_len++], prefix->path[i].node); > + > + path_type = get_type_from_deref(ctx, prefix); > + path_index = index; > + while (!type_is_single_component(path_type)) > + { > + unsigned int next_index = subtype_index_from_component_index(ctx, &path_type, &path_index); > + > + if (!(c = hlsl_new_uint_constant(ctx, next_index, loc))) > + { > + hlsl_free_instr_list(&block->instrs); > + return false; > + } > + list_add_tail(&block->instrs, &c->node.entry); > + > + hlsl_src_from_node(&deref->path[deref_path_len++], &c->node); > + } > + > + assert(deref_path_len == deref->path_len); > + > + return true; > +} > + > struct hlsl_type *hlsl_get_type_from_path_index(struct hlsl_ctx *ctx, const struct hlsl_type *type, > struct hlsl_ir_node *node) > { > @@ -435,6 +605,37 @@ struct hlsl_ir_node *hlsl_new_offset_from_path_index(struct hlsl_ctx *ctx, struc > return idx_offset; > } > > +struct hlsl_ir_node *hlsl_new_offset_node_from_deref(struct hlsl_ctx *ctx, struct hlsl_block *block, > + const struct hlsl_deref *deref, const struct vkd3d_shader_location *loc) For reference, I find the naming of this one okay (aside from node -> instr or something), probably just because of the new_ prefix.

July 28, 2022

Re: [PATCH vkd3d 09/17] vkd3d-shader/hlsl: Replace register offsets with index paths in load initializations.

by Matteo Bruni

On Wed, Jul 20, 2022 at 12:03 AM Zebediah Figura <zfigura(a)codeweavers.com> wrote: > > On 7/19/22 15:21, Francisco Casas wrote: > > Hello, > > > > On 19-07-22 05:11, Giovanni Mascellani wrote: > >> Hi, > >> > >> Il 15/07/22 03:23, Francisco Casas ha scritto: > >>> +/* Given a type and a component index, retrieves next path index > >>> required to reach the component. > >>> + * *typep will be set to the subtype within the original type that > >>> contains the component. > >>> + * *indexp will be set to the index of the component within *typep. > >>> + */ > >>> +static unsigned int subtype_index_from_component_index(struct > >>> hlsl_ctx *ctx, > >>> + struct hlsl_type **typep, unsigned int *indexp) > >> > >> I guess that the "p"'s in "typep" and "indexp" are a sort of reverse > >> Hungarian notation. It's a nitpick, but I am not really a fan of that, > >> and I don't think we're using that anywhere in the HLSL compiler. > >> > > > > I didn't think on Hungarian notation, but I indeed added the 'p' to > > indicate that this is a "pointer to" the actual value. > > > > Because the referenced values (*typep and *indexp) have to be used > > several times, and typep is a double pointer, I assigned these values to > > local variables: > > > > struct hlsl_type *type = *typep; > > unsigned int index = *indexp; > > > > Which is my opinion makes it far more readable that constantly using the > > dereference operator. > > > > The problem is that I had to pick a different name for the local > > variables an the function arguments. > > > > But, unless there is another suggestion, in v3 I am renaming the > > pointers "typep" and "indexp" to "type" and "index" respectively, and > > the values from "type" and "index" to "type_val" and "index_val" > > respectively. > > Personally I prefer the former, and don't particularly mind the -p > suffix. Perhaps "type_ptr" would be more palatable. >From a general naming viewpoint, yes I think it would be. "typep" looks pretty ugly to me...

July 28, 2022

Re: [PATCH vkd3d 07/17] vkd3d-shader/hlsl: Unify type minor/major size functions.

by Matteo Bruni

On Fri, Jul 15, 2022 at 3:24 AM Francisco Casas <fcasas(a)codeweavers.com> wrote: > > Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> > --- > > v2: > * No changes. > > Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> > --- > libs/vkd3d-shader/hlsl.c | 16 ++++++++++++++++ > libs/vkd3d-shader/hlsl.h | 2 ++ > libs/vkd3d-shader/hlsl.y | 20 ++------------------ > libs/vkd3d-shader/hlsl_codegen.c | 28 ++++++---------------------- > 4 files changed, 26 insertions(+), 40 deletions(-) > > diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c > index e4e2ca17..d3ceba35 100644 > --- a/libs/vkd3d-shader/hlsl.c > +++ b/libs/vkd3d-shader/hlsl.c > @@ -124,6 +124,22 @@ bool hlsl_type_is_row_major(const struct hlsl_type *type) > return !!(type->modifiers & HLSL_MODIFIER_ROW_MAJOR); > } > > +unsigned int hlsl_type_minor_size(const struct hlsl_type *type) > +{ > + if (type->type != HLSL_CLASS_MATRIX || hlsl_type_is_row_major(type)) > + return type->dimx; > + else > + return type->dimy; > +} > + > +unsigned int hlsl_type_major_size(const struct hlsl_type *type) > +{ > + if (type->type != HLSL_CLASS_MATRIX || hlsl_type_is_row_major(type)) > + return type->dimy; > + else > + return type->dimx; > +} > + > static unsigned int get_array_size(const struct hlsl_type *type) > { > if (type->type == HLSL_CLASS_ARRAY) > diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h > index 2a6402ce..546c87f3 100644 > --- a/libs/vkd3d-shader/hlsl.h > +++ b/libs/vkd3d-shader/hlsl.h > @@ -799,6 +799,8 @@ unsigned int hlsl_type_get_array_element_reg_size(const struct hlsl_type *type); > unsigned int hlsl_compute_component_offset(struct hlsl_ctx *ctx, struct hlsl_type *type, > unsigned int idx, struct hlsl_type **comp_type); > bool hlsl_type_is_row_major(const struct hlsl_type *type); > +unsigned int hlsl_type_minor_size(const struct hlsl_type *type); > +unsigned int hlsl_type_major_size(const struct hlsl_type *type); > unsigned int hlsl_type_get_sm4_offset(const struct hlsl_type *type, unsigned int offset); > bool hlsl_types_are_equal(const struct hlsl_type *t1, const struct hlsl_type *t2); > > diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y > index b4ca4631..a1d39140 100644 > --- a/libs/vkd3d-shader/hlsl.y > +++ b/libs/vkd3d-shader/hlsl.y > @@ -1216,22 +1216,6 @@ static bool expr_common_shape(struct hlsl_ctx *ctx, struct hlsl_type *t1, struct > return true; > } > > -static unsigned int minor_size(const struct hlsl_type *type) > -{ > - if (type->modifiers & HLSL_MODIFIER_ROW_MAJOR) > - return type->dimx; > - else > - return type->dimy; > -} > - > -static unsigned int major_size(const struct hlsl_type *type) > -{ > - if (type->modifiers & HLSL_MODIFIER_ROW_MAJOR) > - return type->dimy; > - else > - return type->dimx; > -} > - > static struct hlsl_ir_node *add_expr(struct hlsl_ctx *ctx, struct list *instrs, > enum hlsl_ir_expr_op op, struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS], > struct hlsl_type *type, const struct vkd3d_shader_location *loc) > @@ -1247,7 +1231,7 @@ static struct hlsl_ir_node *add_expr(struct hlsl_ctx *ctx, struct list *instrs, > struct hlsl_ir_load *load; > struct hlsl_ir_var *var; > > - vector_type = hlsl_get_vector_type(ctx, type->base_type, minor_size(type)); > + vector_type = hlsl_get_vector_type(ctx, type->base_type, hlsl_type_minor_size(type)); > > name = vkd3d_string_buffer_get(&ctx->string_buffers); > vkd3d_string_buffer_printf(name, "<split_op-%u>", counter++); > @@ -1256,7 +1240,7 @@ static struct hlsl_ir_node *add_expr(struct hlsl_ctx *ctx, struct list *instrs, > if (!var) > return NULL; > > - for (i = 0; i < major_size(type); i++) > + for (i = 0; i < hlsl_type_major_size(type); i++) Nitpick, while at it you could change i++ to ++i as well.

July 28, 2022

Re: [PATCH vkd3d 06/17] vkd3d-shader/hlsl: Introduce add_load_index().

by Matteo Bruni

On Fri, Jul 15, 2022 at 3:24 AM Francisco Casas <fcasas(a)codeweavers.com> wrote: > > At this point add_load() is split into add_load_component() and > add_load_index(); register offsets are hidden for these functions. > > Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> Nice patch. I guess this one is already pretty compelling evidence in support of this "index paths" idea for parse-time IR. > --- > > v2: > * Use "const struct vkd3d_shader_location *loc" instead of "const struct > vkd3d_shader_location loc" > * Use vkd3d_string_buffer for initializing the deref synthetic variable names. > * Move common "load = hlsl_new_load" pattern out of the if..else branches. > * Removed braces on simple switch cases in > hlsl_new_offset_from_path_index(). I kept them in the > HLSL_CLASS_MATRIX case, even though it doesn't have declarations, > because its body seems complex enough. > > Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> > --- > libs/vkd3d-shader/hlsl.c | 105 +++++++++++++++++++++++++++++- > libs/vkd3d-shader/hlsl.h | 7 ++ > libs/vkd3d-shader/hlsl.y | 134 ++++++++++++++------------------------- > 3 files changed, 157 insertions(+), 89 deletions(-) > > diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c > index 2925b4e5..e4e2ca17 100644 > --- a/libs/vkd3d-shader/hlsl.c > +++ b/libs/vkd3d-shader/hlsl.c > @@ -117,7 +117,7 @@ void hlsl_free_var(struct hlsl_ir_var *decl) > vkd3d_free(decl); > } > > -static bool hlsl_type_is_row_major(const struct hlsl_type *type) > +bool hlsl_type_is_row_major(const struct hlsl_type *type) > { > /* Default to column-major if the majority isn't explicitly set, which can > * happen for anonymous nodes. */ > @@ -314,6 +314,109 @@ unsigned int hlsl_compute_component_offset(struct hlsl_ctx *ctx, struct hlsl_typ > return 0; > } > > +struct hlsl_type *hlsl_get_type_from_path_index(struct hlsl_ctx *ctx, const struct hlsl_type *type, > + struct hlsl_ir_node *node) Maybe index, or field, or field_index? In the next function you have "idx", which also works. In any case, pick one and then use consistent naming throughout. Also, I'm a bit confused by the function name: as far as I can see the function has nothing specific to path indices, it's more of a "tell me the type of the next inner component". > +{ > + assert(node); > + > + if (type->type == HLSL_CLASS_VECTOR) > + return hlsl_get_scalar_type(ctx, type->base_type); > + > + if (type->type == HLSL_CLASS_MATRIX) > + { > + if (hlsl_type_is_row_major(type)) > + return hlsl_get_vector_type(ctx, type->base_type, type->dimx); > + else > + return hlsl_get_vector_type(ctx, type->base_type, type->dimy); > + } > + > + if (type->type == HLSL_CLASS_ARRAY) > + return type->e.array.type; > + > + if (type->type == HLSL_CLASS_STRUCT) > + { > + struct hlsl_ir_constant *c = hlsl_ir_constant(node); > + > + assert(c->value[0].u < type->e.record.field_count); > + return type->e.record.fields[c->value[0].u].type; > + } > + > + assert(0); > + return NULL; > +} > + > +struct hlsl_ir_node *hlsl_new_offset_from_path_index(struct hlsl_ctx *ctx, struct hlsl_block *block, > + struct hlsl_type *type, struct hlsl_ir_node *offset, struct hlsl_ir_node *idx, > + const struct vkd3d_shader_location *loc) > +{ > + struct hlsl_ir_node *idx_offset = NULL; > + struct hlsl_ir_constant *c; > + > + list_init(&block->instrs); > + > + switch (type->type) > + { > + case HLSL_CLASS_VECTOR: > + idx_offset = idx; > + break; > + > + case HLSL_CLASS_MATRIX: > + { > + if (!(c = hlsl_new_uint_constant(ctx, 4, loc))) > + return NULL; > + list_add_tail(&block->instrs, &c->node.entry); > + > + if (!(idx_offset = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, &c->node, idx))) > + return NULL; > + list_add_tail(&block->instrs, &idx_offset->entry); > + > + break; > + } > + > + case HLSL_CLASS_ARRAY: > + { > + unsigned int size = hlsl_type_get_array_element_reg_size(type->e.array.type); > + > + if (!(c = hlsl_new_uint_constant(ctx, size, loc))) > + return NULL; > + list_add_tail(&block->instrs, &c->node.entry); > + > + if (!(idx_offset = hlsl_new_binary_expr(ctx, HLSL_OP2_MUL, &c->node, idx))) > + return NULL; > + list_add_tail(&block->instrs, &idx_offset->entry); > + > + break; > + } > + > + case HLSL_CLASS_STRUCT: > + { > + unsigned int field_i = hlsl_ir_constant(idx)->value[0].u; field_idx?

July 28, 2022

Re: [PATCH vkd3d 05/17] vkd3d-shader/hlsl: Introduce add_load_component().

by Matteo Bruni

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

July 28, 2022

Re: [PATCH 3/3] mshtml: Implement "text" type response for XMLHttpRequest.

by Marvin

Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=120063 Your paranoid android. === w10pro64_ja (64 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE === w10pro64_zh_CN (64 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE

July 28, 2022

Re: Access to graphics memory mappings on upcoming WOW64 implementation

by Alexandre Julliard

Derek Lesho <dlesho(a)codeweavers.com> writes: > On 25.04.22 12:38, Derek Lesho wrote: >> >> On 25.04.22 01:31, Zebediah Figura wrote: >>> On 4/24/22 21:18, Derek Lesho wrote: >>>> Hi All, >>>> >>>> In the wake of the new WOW64 implementation (recent explanation >>>> [1]), there has been discussion in informal channels about how to >>>> we are going to handle pointers to mapped graphics resource memory >>>> which we receive from the graphics API, as the possibility exists >>>> that it will fall outside of the 32-bit address space. >>>> >>>> Over time, a few creative solutions have been proposed and >>>> discussed, with a common theme being that we need changes in >>>> either the kernel or the graphics drivers to do this properly. As >>>> we already know the requirements for a solution to this problem, I >>>> think it would be responsible to hash this out now and then work >>>> with the relevant project maintainers earlier as to avoid blocking >>>> work on the wine side too long and to possibly allow more users to >>>> test the new path earlier. >>> >>> Thank you for starting this conversation! I agree with all of these >>> points. WoW64 emulation is still a long way off, if it'll even >>> happen by default on platforms other than Mac, but nevertheless >>> this is something we should look into supporting sooner than later. >>> >>> It would probably be good to start a dri-devel/mesa-dev thread to >>> discuss this as well. >> Agreed, I just filed a feature request at the Vulkan-Docs repo so >> that we can also hear the opinions of those working on non-mesa >> drivers like NV. >> >> https://github.com/KhronosGroup/Vulkan-Docs/issues/1832 > > It looks like Jason Ekstrand drafted two extensions for us here, and > would like to know our opinion on which approach would be the best for > us, as he is even willing to write the extension text for us. > > As explained in the thread, the two approaches are > > 1) Introduce a MAP_32BIT flag to vkMapMemory which the driver would > forward to mmap. > > 2) Read the ppData parameter used to return the memory mapping as a > suggestion for the mapping location, similar to how BaseAddress is > used NtAllocateVirtualMemory. > > I think the more flexible second solution be most ideal for us, as it > allows us to handle the LAA case, but what do you guys think? I don't think that's sufficient, because there's no way to ensure that the address that we picked is still available. What we would want is to have it map into already reserved memory, which would require the driver to use our specified address with MAP_FIXED. Also it would have to avoid calling munmap() on free and let us take care of remapping anonymous memory. -- Alexandre Julliard julliard(a)codeweavers.com

July 28, 2022

Re: [PATCH 2/3] mshtml: Implement responseType prop for XMLHttpRequest.

by Marvin

Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=120062 Your paranoid android. === w10pro64 (64 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS htmldoc.c:350: Test failed: expected Exec_SETTITLE htmldoc.c:2859: Test failed: unexpected call Exec_SETTITLE

July 28, 2022

Re: [PATCH 11/11] mciqtz32: Show the default video window when switching from another one.

by Marvin

Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=120094 Your paranoid android. === w7u_2qxl (32 bit report) === winmm: mci.c:1600: Test failed: 0: window rectangle doesn't match, expected (25,25)-(161,87), got (25,25)-(157,87) mci.c:1600: Test failed: 1: window rectangle doesn't match, expected (100,100)-(236,162), got (100,100)-(232,162) mci.c:1600: Test failed: 2: window rectangle doesn't match, expected (175,175)-(311,237), got (175,175)-(307,237) mci.c:1600: Test failed: 3: window rectangle doesn't match, expected (0,0)-(136,62), got (0,0)-(132,62) mci.c:1600: Test failed: 4: window rectangle doesn't match, expected (125,125)-(261,187), got (125,125)-(257,187) mci.c:1600: Test failed: 5: window rectangle doesn't match, expected (1,1)-(137,63), got (1,1)-(133,63) === w7u_adm (32 bit report) === winmm: mci.c:1600: Test failed: 0: window rectangle doesn't match, expected (100,100)-(236,162), got (100,100)-(232,162) mci.c:1600: Test failed: 1: window rectangle doesn't match, expected (175,175)-(311,237), got (175,175)-(307,237) mci.c:1600: Test failed: 2: window rectangle doesn't match, expected (75,75)-(211,137), got (75,75)-(207,137) mci.c:1600: Test failed: 3: window rectangle doesn't match, expected (0,0)-(136,62), got (0,0)-(132,62) mci.c:1600: Test failed: 4: window rectangle doesn't match, expected (25,25)-(161,87), got (25,25)-(157,87) mci.c:1600: Test failed: 5: window rectangle doesn't match, expected (1,1)-(137,63), got (1,1)-(133,63) === w7u_el (32 bit report) === winmm: mci.c:1600: Test failed: 0: window rectangle doesn't match, expected (75,75)-(211,137), got (75,75)-(207,137) mci.c:1600: Test failed: 1: window rectangle doesn't match, expected (150,150)-(286,212), got (150,150)-(282,212) mci.c:1600: Test failed: 2: window rectangle doesn't match, expected (25,25)-(161,87), got (25,25)-(157,87) mci.c:1600: Test failed: 3: window rectangle doesn't match, expected (0,0)-(136,62), got (0,0)-(132,62) mci.c:1600: Test failed: 4: window rectangle doesn't match, expected (150,150)-(286,212), got (150,150)-(282,212) mci.c:1600: Test failed: 5: window rectangle doesn't match, expected (1,1)-(137,63), got (1,1)-(133,63) === w8 (32 bit report) === winmm: mci.c:1600: Test failed: 0: window rectangle doesn't match, expected (156,156)-(292,219), got (156,156)-(296,219) mci.c:1600: Test failed: 1: window rectangle doesn't match, expected (52,52)-(188,115), got (52,52)-(192,115) mci.c:1600: Test failed: 2: window rectangle doesn't match, expected (130,130)-(266,193), got (130,130)-(270,193) mci.c:1600: Test failed: 3: window rectangle doesn't match, expected (0,0)-(136,63), got (0,0)-(140,63) mci.c:1600: Test failed: 4: window rectangle doesn't match, expected (78,78)-(214,141), got (78,78)-(218,141) mci.c:1600: Test failed: 5: window rectangle doesn't match, expected (1,1)-(137,64), got (1,1)-(141,64) === w8adm (32 bit report) === winmm: mci.c:1600: Test failed: 0: window rectangle doesn't match, expected (156,156)-(292,219), got (156,156)-(296,219) mci.c:1600: Test failed: 1: window rectangle doesn't match, expected (52,52)-(188,115), got (52,52)-(192,115) mci.c:1600: Test failed: 2: window rectangle doesn't match, expected (130,130)-(266,193), got (130,130)-(270,193) mci.c:1600: Test failed: 3: window rectangle doesn't match, expected (0,0)-(136,63), got (0,0)-(140,63) mci.c:1600: Test failed: 4: window rectangle doesn't match, expected (78,78)-(214,141), got (78,78)-(218,141) mci.c:1600: Test failed: 5: window rectangle doesn't match, expected (1,1)-(137,64), got (1,1)-(141,64) === w864 (32 bit report) === winmm: mci.c:1600: Test failed: 0: window rectangle doesn't match, expected (156,156)-(292,219), got (156,156)-(296,219) mci.c:1600: Test failed: 1: window rectangle doesn't match, expected (52,52)-(188,115), got (52,52)-(192,115) mci.c:1600: Test failed: 2: window rectangle doesn't match, expected (130,130)-(266,193), got (130,130)-(270,193) mci.c:1600: Test failed: 3: window rectangle doesn't match, expected (0,0)-(136,63), got (0,0)-(140,63) mci.c:1600: Test failed: 4: window rectangle doesn't match, expected (78,78)-(214,141), got (78,78)-(218,141) mci.c:1600: Test failed: 5: window rectangle doesn't match, expected (1,1)-(137,64), got (1,1)-(141,64) === w864 (64 bit report) === winmm: mci.c:1600: Test failed: 0: window rectangle doesn't match, expected (156,156)-(292,219), got (156,156)-(296,219) mci.c:1600: Test failed: 1: window rectangle doesn't match, expected (52,52)-(188,115), got (52,52)-(192,115) mci.c:1600: Test failed: 2: window rectangle doesn't match, expected (130,130)-(266,193), got (130,130)-(270,193) mci.c:1600: Test failed: 3: window rectangle doesn't match, expected (0,0)-(136,63), got (0,0)-(140,63) mci.c:1600: Test failed: 4: window rectangle doesn't match, expected (78,78)-(214,141), got (78,78)-(218,141) mci.c:1600: Test failed: 5: window rectangle doesn't match, expected (1,1)-(137,64), got (1,1)-(141,64) === debian11 (32 bit report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (32 bit Arabic:Morocco report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (32 bit German report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (32 bit French report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (32 bit Hebrew:Israel report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (32 bit Hindi:India report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (32 bit Japanese:Japan report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (32 bit Chinese:China report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (32 bit WoW report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12) === debian11 (64 bit WoW report) === winmm: mci.c:1657: Test failed: 0: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 1: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 2: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 3: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 4: got (0,0)-(32,24), expected (0,0)-(64,12) mci.c:1657: Test failed: 5: got (0,0)-(32,24), expected (0,0)-(64,12)

July 28, 2022