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

Re: [PATCH vkd3d v3 13/17] vkd3d-shader/hlsl: Replace register offsets with index paths in resource loads initialization.

by Francisco Casas

Ah, I finally see, the hlsl_src_from_node(&deref->offset, other->offset.node); indeed is a noop. I will fix it. On 25-07-22 04:11, Giovanni Mascellani wrote: > Hi, > > Il 22/07/22 20:36, Francisco Casas ha scritto: >>>> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c >>>> index 2d5fe40a..a4d594b7 100644 >>>> --- a/libs/vkd3d-shader/hlsl.c >>>> +++ b/libs/vkd3d-shader/hlsl.c >>>> @@ -862,6 +862,28 @@ static bool type_is_single_reg(const struct >>>> hlsl_type *type) >>>> return type->type == HLSL_CLASS_SCALAR || type->type == >>>> HLSL_CLASS_VECTOR; >>>> } >>>> +static bool deref_copy(struct hlsl_ctx *ctx, struct hlsl_deref >>>> *deref, struct hlsl_deref *other) >>>> +{ >>>> + unsigned int i; >>>> + >>>> + memset(deref, 0, sizeof(*deref)); >>>> + >>>> + if (!other) >>>> + return true; >>>> + >>>> + assert(!other->offset.node); >>>> + >>>> + if (!init_deref(ctx, deref, other->var, other->path_len)) >>>> + return false; >>>> + >>>> + for (i = 0; i < deref->path_len; ++i) >>>> + hlsl_src_from_node(&deref->path[i], other->path[i].node); >>>> + >>>> + hlsl_src_from_node(&deref->offset, other->offset.node); >>> >>> You just asserted that other->offset.node is NULL and *deref was >>> initialized to zero, so that seems to be a noop, doesn't it? >>> >> >> Hmm, sorry, I am not sure I understand. >> Initializing *deref to zero shouldn't affect >> >> assert(!other->offset.node); >> >> The idea of this assertion is to ensure that the "other" deref is in >> the index path format instead of the register offset format. > > My point is that, given that you just asserted that other->offset.node > is NULL, the effect of "hlsl_src_from_node(&deref->offset, > other->offset.node)" would be to set deref->offset.node to NULL, but > that's already ensured by the memset(). Or maybe I am getting confused > on something trivial? > > Giovanni.

Aug. 4, 2022

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

by Matteo Bruni

On Thu, Aug 4, 2022 at 9:10 PM Francisco Casas <fcasas(a)codeweavers.com> wrote: > > Hello, > > On 28-07-22 16:59, Matteo Bruni wrote: > > 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). > > I tried it passing a hlsl_blocks to be initialized (like in the > component load/store initializers) but in the end it didn't seem like a > good idea to me: > > The problem is that we would need to create derefs in the wild (outside > hlsl_ir_nodes), which would mean exposing several static functions in > hlsl.c, like init_deref_from_component_index(). We would also need to > use "hlsl_cleanup_deref()" which for now is only non-static because it > is needed in the the pass that translates index paths back to register > offsets. > > So I don't think exposing the deref functions is better than adding > these redundant loads, which are later deleted by the DCE pass, if I am > not mistaken. > > Alternatively, we could create the nodes just as a container for the > derefs and immediately free them, but that also seems a little ugly. > > So my vote is to keep this signature. Maybe remove the "type" parameter > since it can be obtained from the deref inside the load node. Fair enough. That was hard to see without actually trying and it sounds like redundant loads are the lesser evil in this case. Maybe a comment somewhere explaining that these loads are redundant and supposed to be eliminated by DCE would be nice.

Aug. 4, 2022

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

by Francisco Casas

Hello, On 28-07-22 16:59, Matteo Bruni wrote: > 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). I tried it passing a hlsl_blocks to be initialized (like in the component load/store initializers) but in the end it didn't seem like a good idea to me: The problem is that we would need to create derefs in the wild (outside hlsl_ir_nodes), which would mean exposing several static functions in hlsl.c, like init_deref_from_component_index(). We would also need to use "hlsl_cleanup_deref()" which for now is only non-static because it is needed in the the pass that translates index paths back to register offsets. So I don't think exposing the deref functions is better than adding these redundant loads, which are later deleted by the DCE pass, if I am not mistaken. Alternatively, we could create the nodes just as a container for the derefs and immediately free them, but that also seems a little ugly. So my vote is to keep this signature. Maybe remove the "type" parameter since it can be obtained from the deref inside the load node.

Aug. 4, 2022

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

by Francisco Casas

Hello, On 28-07-22 17:00, Matteo Bruni wrote: >> static bool split_copy(struct hlsl_ctx *ctx, struct hlsl_ir_store *store, >> - const struct hlsl_ir_load *load, const unsigned int offset, struct hlsl_type *type) >> + const struct hlsl_ir_load *load, const unsigned int indx, struct hlsl_type *type) > > Why "indx" now? The rename is because I don't want to get it confused with "offset" as in "register offset", since it is a "component index" now. In case you meant why I wrote "indx" instead of "idx", which seems to be the standard, it was just a mistake, I will change it.

Aug. 4, 2022

Re: [PATCH v8 2/2] win32u/window: Ignore changing WS_EX_TOPMOST for message-only windows.

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=120511 Your paranoid android. === w10pro64 (32 bit report) === user32: win.c:10984: Test failed: pos = 014a015e

Aug. 4, 2022

Re: [PATCH v8 1/2] user32/tests: Check that message-only windows ignore WS_EX_TOPMOST.

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=120510 Your paranoid android. === w10pro64_en_AE_u8 (64 bit report) === user32: win.c:4592: Test failed: hwnd 00000000001F0326/00000000001F0326 message 0200 win.c:4595: Test failed: hwnd 00000000001F0326/00000000001F0326 message 0201 === w10pro64_ar (64 bit report) === user32: win.c:4641: Test failed: hwnd 00000000000B02D2/00000000000B02D2 message 0200 win.c:4643: Test failed: wparam 0 win.c:4654: Test failed: hwnd 00000000000B02D2/00000000000B02D2 message 0203 win.c:4658: Test failed: message 0202 available

Aug. 4, 2022

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

by Giovanni Mascellani

Il 02/08/22 22:25, Matteo Bruni ha scritto: >> BTW, an alternative name for the function could be >> 'traverse_path_from_component_index', but I am not totally sure. > Also good, it feels like an improvement to me. > > I wonder what Zeb and Giovanni think about this (if anything) 😄 No specific thought, so it's good. Giovanni.

Aug. 4, 2022

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

by Matteo Bruni

On Wed, Aug 3, 2022 at 11:26 PM Francisco Casas <fcasas(a)codeweavers.com> wrote: > > > > On 28-07-22 16:44, Matteo Bruni wrote: > > Okay, I guess I managed to read the patch series through. Sorry it > > took me way longer than I'd hoped, and I didn't even properly review > > the individual patches (e.g. apply them and check that the tests pass > > after each one). > > > > In general the patch series seems okay to me and, as far as I'm > > concerned, if this goes in with no changes at all I would have no > > qualms. > > > > That said, I do have a number of relatively minor comments that I'll > > send in reply to individual patches. For the earlier patches in the > > series (like this one) I'm going to reply to "v2" since that's where I > > had started to write them, but I don't think anything mentioned in > > those was affected by the changes in v3. > > > > On Fri, Jul 15, 2022 at 3:24 AM Francisco Casas <fcasas(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. > > Understood. However, in this particular case (and later, > add_load_index()) I can't think of a better name for the node argument, > besides "value". "Instr" should always work as a replacement. Node really isn't an appropriate name anymore, specifically since we flattened the IR a while back.

Aug. 3, 2022

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

by Francisco Casas

On 28-07-22 16:44, Matteo Bruni wrote: > Okay, I guess I managed to read the patch series through. Sorry it > took me way longer than I'd hoped, and I didn't even properly review > the individual patches (e.g. apply them and check that the tests pass > after each one). > > In general the patch series seems okay to me and, as far as I'm > concerned, if this goes in with no changes at all I would have no > qualms. > > That said, I do have a number of relatively minor comments that I'll > send in reply to individual patches. For the earlier patches in the > series (like this one) I'm going to reply to "v2" since that's where I > had started to write them, but I don't think anything mentioned in > those was affected by the changes in v3. > > On Fri, Jul 15, 2022 at 3:24 AM Francisco Casas <fcasas(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. Understood. However, in this particular case (and later, add_load_index()) I can't think of a better name for the node argument, besides "value".

Aug. 3, 2022

Re: [PATCH 2/2] mshtml: Implement targetOrigin for postMessage.

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=120464 Your paranoid android. === w10pro64_en_AE_u8 (64 bit report) === mshtml: htmldoc.c:2541: Test failed: unexpected call UpdateUI htmldoc.c:2853: Test failed: unexpected call Exec_UPDATECOMMANDS

Aug. 3, 2022