May 2017 - Wine-Devel - list.winehq.org

Re: [PATCH 1/6] d3dx9: Get rid of table lookup for converting between register indexes and offsets.

by Paul Gofman

On 05/30/2017 01:59 AM, Matteo Bruni wrote: > >>>> @@ -1033,9 +1035,9 @@ static void set_constants(struct d3dx_regstore *rs, >>>> struct d3dx_const_tab *const >>>> unsigned int offset; >>>> >>>> offset = start_offset + i * major_stride + j; >>>> - if (offset / table_info[table].reg_component_count >= >>>> rs->table_sizes[table]) >>>> + if (get_reg_offset(table, offset) >= >>>> rs->table_sizes[table]) >>>> { >>>> - if (table_info[table].reg_component_count != 1) >>>> + if (table != PRES_REGTAB_OBCONST) >>>> FIXME("Output offset exceeds table size, name >>>> %s, component %u.\n", >>>> debugstr_a(param->name), i); >>>> break; >>> Not new and probably I just forgot the details, but, why would the >>> register component count matter for the FIXME? >> The length of the data to be copied to registers can be limited by >> RegisterCount in (pre)shader constant description. But when register is >> 4-value, the major count for output loop should already mind that (since we >> always have row count and column count <= 4). The only legitimate case we >> may need to break from the inner loop is boolean constant matrix setting. So >> actually fixme condition should never happen, though break from the inner >> loop on boolean constant setting may be required. I am changing this place >> in the next patches (I didn't send yet) when optimizing this place. I was >> actually going to add a bit more tests for this case along with the >> forthcoming changes, and I suspect a bug here (I actually need to >> effectively compare with constant register count and not with the table >> size). We have a test for full update with boolean matrices (which will not >> trigger the error here) but commit of individual matrices test probably >> will. > Right, that makes sense, including the likely bug you mention. > > In theory it should be possible to do the FIXME check at init time. > Actually, the whole constant upload info could be precomputed. Just > putting it out there... > My plan was actually to get rid of this FIXME at all (it is actually more of a debug assertion rather than true FIXME which seems redundant now), and besides introduce more code paths for matrix settings, like e. g. the cases when output stride is the same as input. I strictly want to avoid any 'if' in the inner loops at least. Ideally I would maybe like to end up with strides values specialized loops for possible matrix sizes, but not sure yet if it worth it. Regarding precomputing matrix strides / counts, I tried to store them all in const_set structure (after the optimizations I mentioned), but did not see any improvement. It takes quite a lot of fields and probably getting them on the fly takes not more time than adding much more data to const_set.

May 30, 2017

Re: [PATCH] macdrv: Don't update clipboard if its content didn't change

by Piotr Caban

On 05/30/17 06:40, Ken Thomases wrote: > Since the vast majority of apps will call -clearContents or -declareTypes:owner: each time they modify the pasteboard, I'm willing to accept a version of this change. Do you mind restoring the tracking and checking of last_types, so that we grab the Win32 clipboard if either the change count or the available types has changed? Sure, I'll send a new patch. Thanks, Piotr

May 30, 2017

Re: [PATCH] macdrv: Don't update clipboard if its content didn't change

by Ken Thomases

On May 26, 2017, at 1:23 AM, Piotr Caban <piotr(a)codeweavers.com> wrote: > >> I'll have to run some tests again to see exactly which operations do and don't increment the change count. I'll let you know what I find. >> >> It may be that your patch is better for the vast majority of the cases such that we should commit it. We'll just accept incorrect behavior for the (hopefully) rare aberrant uses of the API that I'm concerned about. Or maybe we can take a belt-and-suspenders approach and check both the change count and the list of types. > > I’ve done some initial testing: > > [myPasteboard clearContents]; > clearContent increments the changeCount. > > [myPasteboard setString:@"test" forType:NSStringPboardType]; > NSLog(@"changeCount %ld", [myPasteboard changeCount]); > [NSThread sleepForTimeInterval:2]; > [myPasteboard setString:@"test2" forType:NSStringPboardType]; > NSLog(@“changeCount %ld", [myPasteboard changeCount]); > In this case changeCount doesn’t change no matter if other applications read the pasteboard data between setString calls. > > [myPasteboard addTypes:[NSArray arrayWithObject:NSRTFPboardType] owner:nil]; > NSLog(@“addTypes changeCount %ld”, [myPasteboard changeCount]); > [myPasteboard setString:rtf forType:NSRTFPboardType]; > NSLog(@"new data type changeCount %ld", [myPasteboard changeCount]); > Adding new clipboard format doesn’t change changeCount. Setting value for newly added format also doesn’t change it. > > [myPasteboard declareTypes:[NSArray arrayWithObjects:NSStringPboardType, NSURLPboardType, nil] owner:nil]; > declareTypes changes changeCount value. Thanks for running those tests. They seem to confirm what I remembered. Since the vast majority of apps will call -clearContents or -declareTypes:owner: each time they modify the pasteboard, I'm willing to accept a version of this change. Do you mind restoring the tracking and checking of last_types, so that we grab the Win32 clipboard if either the change count or the available types has changed? If this approach causes problems in the future, we'll reconsider at that time. Thanks, Ken

May 30, 2017

Re: [PATCH 6/6] d3dx9: Merge constant setting for child parameters when possible.

by Matteo Bruni

2017-05-29 20:44 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 05/29/2017 09:24 PM, Matteo Bruni wrote: >> >> 2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >> >>> + if (!(const_set->table == current_table && >>> current_start_offset == start_offset >>> + && const_set->direct_copy == >>> first_const->direct_copy >>> + && current_data == const_set->param->data >>> + && (const_set->direct_copy || >>> (first_const->param->type == const_set->param->type >>> + && first_const->param->class == >>> const_set->param->class >>> + && first_const->param->columns == >>> const_set->param->columns >>> + && first_const->param->rows == >>> const_set->param->rows >>> + && first_const->register_count == >>> const_set->register_count >>> + && (i == const_tab->const_set_count - 1 >>> + || first_const->param->element_count == >>> const_set->param->element_count))))) >>> + { >>> + TRACE("direct_copy %u, i %u, index %u, param %s.%s, >>> current_data %p, const_set->param->data %p, "\ >>> + "current_start_offset %u, start_offset %u, >>> const_set->table %u, current_table %u.\n", >>> + const_set->direct_copy, i, index, >>> debugstr_a(param->name), >>> + debugstr_a(const_set->param->name), >>> current_data, const_set->param->data, >>> + current_start_offset, start_offset, >>> const_set->table, current_table); >>> + break; >>> + } >> >> This looks like a debug trace. >> >>> + TRACE("Merging %u child parameters for %s, not merging %u, >>> direct_copy %u.\n", i - index, >>> + debugstr_a(param->name), const_tab->const_set_count - i, >>> first_const->direct_copy); >> >> I guess it's intentional that you're printing this even when not >> merging anything, in that case the TRACE sounds a bit awkward though. >> Maybe have a separate TRACE for that case? > > Both traces are to have a possibility to check from trace log what is merged > and not "merged" and why, for performance analysis. If you think it is more > of debug I can remove it, though since it is in initialization only I was > thinking it does not add too much noise. The first trace feels pretty noisy to me for upstream Wine, do you think it would be useful in a trace attached on a generic bug to figure out potential performance improvements? The "Merging" one is fine, I'm just a tiny bit annoyed by the "Merged 0 child" case. >> Just for my own curiosity, do you think it's significant to merge >> non-direct_copy entries? Not that it hurts anything to have it. >> >> > Yes, it is crucial together with the next patches I did not send yet. Even > in this patchset removing extra const_set entries is beneficial: it removes > a lot of dirty checks when skipping const_set's from the same parameter on > CommitChanges, and also initializing the data for inner loops less times. In > the next patches which is not there yet I: > > - Factor out type conversion which can be used for an array of values, > removing switching for each value being set; > > - Introduce a separate path for setting scalar and vectors (which is simpler > than general matrix case an quite often may be grouped effectively even when > it is not 'direct_copy'); > > - Optimize matrix settings loops. > > Copying ~100 element array with transpose is quite a common case. > Merging them in a one const_set and doing that things further is a huge > optimization actually. Cool. Optimizing the transposed matrix case is probably worthy by itself.

May 29, 2017

Re: [PATCH 4/6] d3dx9: Evaluate condition if parameter can be directly copied during initialization.

by Matteo Bruni

2017-05-29 20:35 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 05/29/2017 09:21 PM, Matteo Bruni wrote: >> >> 2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >> >> The patch subject is a bit cumbersome IMO, what about "d3dx9: Add a >> direct copy path to set_constants()."? >> > But direct copy path is already there, I am just moving lengthy condition > evaluation for that to the initialization where it is stored in the > structure (and this is used during initialization also in the next patch). > Maybe "d3dx9: Add a direct copy condition to struct param_eval_output"? Eh, right. Dunno, "d3dx9: Precompute and store a constant direct copy flag."?

May 29, 2017

Re: [PATCH 1/6] d3dx9: Get rid of table lookup for converting between register indexes and offsets.

by Matteo Bruni

2017-05-29 20:32 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > On 05/29/2017 09:19 PM, Matteo Bruni wrote: >> >> 2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >> >>> static unsigned int get_reg_offset(unsigned int table, unsigned int >>> offset) >>> { >>> - return offset / table_info[table].reg_component_count; >>> + return table == PRES_REGTAB_OBCONST ? offset : offset >> 2; >>> +} >>> + >>> +static unsigned int get_offset_reg(unsigned int table, unsigned int >>> reg_idx) >>> +{ >>> + return table == PRES_REGTAB_OBCONST ? reg_idx : reg_idx << 2; >>> } >> >> I suppose using an explicit division and multiplication here wouldn't >> change the generated code, in which case I'd slightly prefer that. It >> doesn't matter right now, just mentioning it for potentially similar >> cases in the future. > > Yes, sure, if it would be constant literal for multiplication / division > here. Yeah, it should be a trivial optimization for the compiler and a tiny bit clearer for the reader. Not a big deal of course. >>> @@ -1033,9 +1035,9 @@ static void set_constants(struct d3dx_regstore *rs, >>> struct d3dx_const_tab *const >>> unsigned int offset; >>> >>> offset = start_offset + i * major_stride + j; >>> - if (offset / table_info[table].reg_component_count >= >>> rs->table_sizes[table]) >>> + if (get_reg_offset(table, offset) >= >>> rs->table_sizes[table]) >>> { >>> - if (table_info[table].reg_component_count != 1) >>> + if (table != PRES_REGTAB_OBCONST) >>> FIXME("Output offset exceeds table size, name >>> %s, component %u.\n", >>> debugstr_a(param->name), i); >>> break; >> >> Not new and probably I just forgot the details, but, why would the >> register component count matter for the FIXME? > > The length of the data to be copied to registers can be limited by > RegisterCount in (pre)shader constant description. But when register is > 4-value, the major count for output loop should already mind that (since we > always have row count and column count <= 4). The only legitimate case we > may need to break from the inner loop is boolean constant matrix setting. So > actually fixme condition should never happen, though break from the inner > loop on boolean constant setting may be required. I am changing this place > in the next patches (I didn't send yet) when optimizing this place. I was > actually going to add a bit more tests for this case along with the > forthcoming changes, and I suspect a bug here (I actually need to > effectively compare with constant register count and not with the table > size). We have a test for full update with boolean matrices (which will not > trigger the error here) but commit of individual matrices test probably > will. Right, that makes sense, including the likely bug you mention. In theory it should be possible to do the FIXME check at init time. Actually, the whole constant upload info could be precomputed. Just putting it out there...

May 29, 2017

Re: [PATCH v7 09/26] x86/insn-eval: Add utility function to identify string instructions

by Borislav Petkov

On Fri, May 05, 2017 at 11:17:07AM -0700, Ricardo Neri wrote: > String instructions are special because in protected mode, the linear > address is always obtained via the ES segment register in operands that > use the (E)DI register. ... and DS for rSI. If we're going to account for both operands of string instructions with two operands. Btw, LODS and OUTS use only DS:rSI as a source operand. So we have to be careful with the generalization here. So if ES:rDI is the only seg. reg we want, then we don't need to look at those insns... (we assume DS by default). ... > +/** > + * is_string_instruction - Determine if instruction is a string instruction > + * @insn: Instruction structure containing the opcode > + * > + * Return: true if the instruction, determined by the opcode, is any of the > + * string instructions as defined in the Intel Software Development manual. > + * False otherwise. > + */ > +static bool is_string_instruction(struct insn *insn) > +{ > + insn_get_opcode(insn); > + > + /* all string instructions have a 1-byte opcode */ > + if (insn->opcode.nbytes != 1) > + return false; > + > + switch (insn->opcode.bytes[0]) { > + case INSB: > + /* fall through */ > + case INSW_INSD: > + /* fall through */ > + case OUTSB: > + /* fall through */ > + case OUTSW_OUTSD: > + /* fall through */ > + case MOVSB: > + /* fall through */ > + case MOVSW_MOVSD: > + /* fall through */ > + case CMPSB: > + /* fall through */ > + case CMPSW_CMPSD: > + /* fall through */ > + case STOSB: > + /* fall through */ > + case STOSW_STOSD: > + /* fall through */ > + case LODSB: > + /* fall through */ > + case LODSW_LODSD: > + /* fall through */ > + case SCASB: > + /* fall through */ That "fall through" for every opcode is just too much. Also, you can use the regularity of the x86 opcode space and do: case 0x6c ... 0x6f: /* INS/OUTS */ case 0xa4 ... 0xa7: /* MOVS/CMPS */ case 0xaa ... 0xaf: /* STOS/LODS/SCAS */ return true; default: return false; } And voila, there's your compact is_string_insn() function! :^) (Modulo the exact list, as I mentioned above). Thanks. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --

May 29, 2017

Re: [PATCH 2/7] msvcrt: Clean up registered C++ object in handler.

by Piotr Caban

On 05/24/17 02:55, Daniel Lehman wrote: > +static DWORD cxx_catch_cleanup(EXCEPTION_RECORD *rec, EXCEPTION_REGISTRATION_RECORD *frame, > + CONTEXT *context, EXCEPTION_REGISTRATION_RECORD **pdispatcher) > +{ > + if (rec->ExceptionFlags & (EH_UNWINDING | EH_EXIT_UNWIND)) > + { > + thread_data_t *data = msvcrt_get_thread_data(); > + frame_info *cur; > + > + if (cxx_is_consolidate(rec)) Is this condition really needed? Shouldn't we clean the object no matter what's the reason of unwind? > + { > + rec = (void*)rec->ExceptionInformation[4]; > + > + for (cur = data->frame_info_head; cur; cur = cur->next) > + { > + if ((ULONG64)cur <= (ULONG64)frame) This condition is not working. It's making assumption about order of catch_frame and frame_info variables on stack while they are declared this way: > + EXCEPTION_REGISTRATION_RECORD catch_frame; > cxx_frame_info frame_info; Shouldn't the cxx_catch_cleanup just unregister the object that was registered in call_catch_block? Thanks, Piotr

May 29, 2017

Re: [PATCH 6/6] d3dx9: Merge constant setting for child parameters when possible.

by Paul Gofman

On 05/29/2017 09:24 PM, Matteo Bruni wrote: > 2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > >> + if (!(const_set->table == current_table && current_start_offset == start_offset >> + && const_set->direct_copy == first_const->direct_copy >> + && current_data == const_set->param->data >> + && (const_set->direct_copy || (first_const->param->type == const_set->param->type >> + && first_const->param->class == const_set->param->class >> + && first_const->param->columns == const_set->param->columns >> + && first_const->param->rows == const_set->param->rows >> + && first_const->register_count == const_set->register_count >> + && (i == const_tab->const_set_count - 1 >> + || first_const->param->element_count == const_set->param->element_count))))) >> + { >> + TRACE("direct_copy %u, i %u, index %u, param %s.%s, current_data %p, const_set->param->data %p, "\ >> + "current_start_offset %u, start_offset %u, const_set->table %u, current_table %u.\n", >> + const_set->direct_copy, i, index, debugstr_a(param->name), >> + debugstr_a(const_set->param->name), current_data, const_set->param->data, >> + current_start_offset, start_offset, const_set->table, current_table); >> + break; >> + } > This looks like a debug trace. > >> + TRACE("Merging %u child parameters for %s, not merging %u, direct_copy %u.\n", i - index, >> + debugstr_a(param->name), const_tab->const_set_count - i, first_const->direct_copy); > I guess it's intentional that you're printing this even when not > merging anything, in that case the TRACE sounds a bit awkward though. > Maybe have a separate TRACE for that case? Both traces are to have a possibility to check from trace log what is merged and not "merged" and why, for performance analysis. If you think it is more of debug I can remove it, though since it is in initialization only I was thinking it does not add too much noise. > > BTW, please format the BOOL with %#x. > >> + if (i > index + 1) >> + { >> + first_const->element_count = element_count; >> + if (first_const->direct_copy) >> + { >> + first_const->element_count = 1; >> + if (index == start_index >> + && !(param->type == D3DXPT_VOID && param->class == D3DXPC_STRUCT)) >> + { >> + if (param_type_to_table_type(param->type) == PRES_VT_MAX) >> + return D3DERR_INVALIDCALL; >> + first_const->param = param; >> + } >> + first_const->register_count = get_reg_offset(current_table, current_start_offset) >> + - first_const->register_index; >> + } >> + memmove(&const_tab->const_set[index + 1], &const_tab->const_set[i], >> + sizeof(*const_tab->const_set) * (const_tab->const_set_count - i)); >> + const_tab->const_set_count -= i - index - 1; >> + } > Just for my own curiosity, do you think it's significant to merge > non-direct_copy entries? Not that it hurts anything to have it. > > Yes, it is crucial together with the next patches I did not send yet. Even in this patchset removing extra const_set entries is beneficial: it removes a lot of dirty checks when skipping const_set's from the same parameter on CommitChanges, and also initializing the data for inner loops less times. In the next patches which is not there yet I: - Factor out type conversion which can be used for an array of values, removing switching for each value being set; - Introduce a separate path for setting scalar and vectors (which is simpler than general matrix case an quite often may be grouped effectively even when it is not 'direct_copy'); - Optimize matrix settings loops. Copying ~100 element array with transpose is quite a common case. Merging them in a one const_set and doing that things further is a huge optimization actually.

May 29, 2017

Re: [PATCH 4/6] d3dx9: Evaluate condition if parameter can be directly copied during initialization.

by Paul Gofman

On 05/29/2017 09:21 PM, Matteo Bruni wrote: > 2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > > The patch subject is a bit cumbersome IMO, what about "d3dx9: Add a > direct copy path to set_constants()."? > But direct copy path is already there, I am just moving lengthy condition evaluation for that to the initialization where it is stored in the structure (and this is used during initialization also in the next patch). Maybe "d3dx9: Add a direct copy condition to struct param_eval_output"?

May 29, 2017