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

HLSL compiler change log

by Giovanni Mascellani

Hi, I tried to put together a quick change log for the HLSL compiler since vkd3d 1.3. I concentrated on user-visible changes (so mostly ignoring tests and internal refactoring), a possible side effect of which is that my contributions are somehow more evident. I don't mean to belittle the work of who did more plumbing job. --- * Support integer arithmetic, bitwise and shift operations. * Fold constant operations for a number of operators. * Support matrix and vector subscripting. * Support matrix copying, casting, entry-wise operations and row-by-column multiplication. * Support complex initializers. * Support the "nointerpolation" modifier, and assume it for integer variables. * Perform copy propagation even when the code has branches and/or loops. --- Francisco, are there relevant cases of complex initializers that are still unsupported in master? Anything else I skipped? Thanks, Giovanni.

June 21, 2022

Re: [PATCH 3/3] d2d1: Add stubs for ID2D1TransformGraph.

by Nikolay Sivov

On 6/21/22 06:17, Ziqing Hui wrote: > + > +err: > + if (effect->impl) > + ID2D1EffectImpl_Release(effect->impl); > + free(transform_graph); > + return hr; Graph object freeing does not look right here.

June 21, 2022

Re: [PATCH 2/3] d2d1: Implement effect creation for registered effect.

by Nikolay Sivov

On 6/21/22 06:17, Ziqing Hui wrote: > HRESULT d2d_effect_init(struct d2d_effect *effect, struct d2d_effect_context *effect_context, const CLSID *effect_id) > { > - unsigned int i; > + struct d2d_effect_registration *reg; > + struct d2d_factory *factory; > + HRESULT hr; > > effect->ID2D1Effect_iface.lpVtbl = &d2d_effect_vtbl; > effect->ID2D1Image_iface.lpVtbl = &d2d_effect_image_vtbl; > effect->refcount = 1; > > - for (i = 0; i < ARRAY_SIZE(builtin_effects); ++i) > + factory = unsafe_impl_from_ID2D1Factory3((ID2D1Factory3 *)effect_context->device_context->factory); > + LIST_FOR_EACH_ENTRY(reg, &factory->effects, struct d2d_effect_registration, entry) > { > - if (IsEqualGUID(effect_id, &builtin_effects[i].id)) > + if (IsEqualGUID(effect_id, &reg->id)) > { > - effect->min_inputs = builtin_effects[i].min_inputs; > - effect->max_inputs = builtin_effects[i].max_inputs; > - d2d_effect_SetInputCount(&effect->ID2D1Effect_iface, builtin_effects[i].default_input_count); > + if (FAILED(hr = reg->factory((IUnknown **)&effect->impl))) > + return hr; > + if (FAILED(hr = ID2D1EffectImpl_Initialize(effect->impl, &effect_context->ID2D1EffectContext_iface, NULL))) > + { > + ID2D1EffectImpl_Release(effect->impl); > + return hr; > + } > + effect->id = *effect_id; > + effect->min_inputs = reg->min_inputs; > + effect->max_inputs = reg->max_inputs; > + d2d_effect_SetInputCount(&effect->ID2D1Effect_iface, reg->default_input_count); > effect->effect_context = effect_context; > ID2D1EffectContext_AddRef(&effect_context->ID2D1EffectContext_iface); > + /* FIXME: Properties are ignored. */ > return S_OK; > } > } CreateEffect() is a single place where this would be used, so I'd suggest to pick correct d2d_effect_registration there, and simply pass it to this helper. Using unsafe_* is normally reserved for externally provided pointers, that's not the case here. > static const struct d2d_builtin_effect_registration builtin_effects[] = > { > - {&CLSID_D2D12DAffineTransform, NULL, 1, 1, 1}, > - {&CLSID_D2D13DPerspectiveTransform, NULL, 1, 1, 1}, > - {&CLSID_D2D1Composite, NULL, 2, 1, 0xffffffff}, > - {&CLSID_D2D1Crop, NULL, 1, 1, 1}, > - {&CLSID_D2D1Shadow, NULL, 1, 1, 1}, > - {&CLSID_D2D1Grayscale, NULL, 1, 1, 1}, > + {&CLSID_D2D12DAffineTransform, d2d_effect_impl_create, 1, 1, 1}, > + {&CLSID_D2D13DPerspectiveTransform, d2d_effect_impl_create, 1, 1, 1}, > + {&CLSID_D2D1Composite, d2d_effect_impl_create, 2, 1, 0xffffffff}, > + {&CLSID_D2D1Crop, d2d_effect_impl_create, 1, 1, 1}, > + {&CLSID_D2D1Shadow, d2d_effect_impl_create, 1, 1, 1}, > + {&CLSID_D2D1Grayscale, d2d_effect_impl_create, 1, 1, 1}, > }; I don't know how extensible this is, every _create would be different.

June 21, 2022

Re: [PATCH 1/3] d2d1: Support registering builtin effect.

by Nikolay Sivov

On 6/21/22 06:17, Ziqing Hui wrote: > @@ -616,7 +651,9 @@ struct d2d_effect > ID2D1Image ID2D1Image_iface; > LONG refcount; > > - const struct d2d_effect_info *info; > + CLSID id; > + UINT32 min_inputs; > + UINT32 max_inputs; > > struct d2d_effect_context *effect_context; > ID2D1Image **inputs; This should be a part of property system, not exposed like that. > @@ -554,21 +563,21 @@ static HRESULT STDMETHODCALLTYPE d2d_effect_GetValue(ID2D1Effect *iface, UINT32 > { > case D2D1_PROPERTY_CLSID: > if ((type != D2D1_PROPERTY_TYPE_UNKNOWN && type != D2D1_PROPERTY_TYPE_CLSID) > - || value_size != sizeof(*effect->info->clsid)) > + || value_size != sizeof(effect->id)) > return E_INVALIDARG; > - src = effect->info->clsid; > + src = &effect->id; > break; > case D2D1_PROPERTY_MIN_INPUTS: > if ((type != D2D1_PROPERTY_TYPE_UNKNOWN && type != D2D1_PROPERTY_TYPE_UINT32) > - || value_size != sizeof(effect->info->min_inputs)) > + || value_size != sizeof(effect->min_inputs)) > return E_INVALIDARG; > - src = &effect->info->min_inputs; > + src = &effect->min_inputs; > break; > case D2D1_PROPERTY_MAX_INPUTS: > if ((type != D2D1_PROPERTY_TYPE_UNKNOWN && type != D2D1_PROPERTY_TYPE_UINT32) > - || value_size != sizeof(effect->info->max_inputs)) > + || value_size != sizeof(effect->max_inputs)) > return E_INVALIDARG; > - src = &effect->info->max_inputs; > + src = &effect->max_inputs; > break; > default: > if (index < D2D1_PROPERTY_CLSID) Similarly, this needs rework. > -static const struct d2d_effect_info builtin_effects[] = > +struct d2d_builtin_effect_registration > { > - {&CLSID_D2D12DAffineTransform, 1, 1, 1}, > - {&CLSID_D2D13DPerspectiveTransform, 1, 1, 1}, > - {&CLSID_D2D1Composite, 2, 1, 0xffffffff}, > - {&CLSID_D2D1Crop, 1, 1, 1}, > - {&CLSID_D2D1Shadow, 1, 1, 1}, > - {&CLSID_D2D1Grayscale, 1, 1, 1}, > + const CLSID *id; > + PD2D1_EFFECT_FACTORY factory; > + UINT32 default_input_count; > + UINT32 min_inputs; > + UINT32 max_inputs; > +}; It should be possible to reuse same structure for builtin effects. > +HRESULT d2d_register_builtin_effects(struct d2d_factory *factory) > +{ > + struct d2d_effect_registration *reg; > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(builtin_effects); ++i) > + { > + const struct d2d_builtin_effect_registration *builtin_reg = &builtin_effects[i]; > + > + if (!(reg = calloc(1, sizeof(*reg)))) > + return E_OUTOFMEMORY; > + > + reg->is_builtin = TRUE; > + reg->factory = builtin_reg->factory; > + reg->registration_count = 1; > + reg->id = *builtin_reg->id; > + reg->default_input_count = builtin_reg->default_input_count; > + reg->min_inputs = builtin_reg->min_inputs; > + reg->max_inputs = builtin_reg->max_inputs; > + list_add_tail(&factory->effects, &reg->entry); > + } > + > + return S_OK; > +} I'm not sure if we want that. It should be enough to check if CLSID is for builtin in Register* call, and redirect to builtin data in CreateEffect().

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Again, I generally like the MR. Nevertheless, I've got a bunch of comments... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2446

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c: > + > +static struct data_object* data_object_from_ID3DX10DataLoader(ID3DX10DataLoader *iface) > +{ > + return CONTAINING_RECORD(iface, struct data_object, ID3DX10DataLoader_iface); > +} > + > +static LONG data_loader_load_count; > +static WINAPI HRESULT data_loader_Load(ID3DX10DataLoader *iface) > +{ > + struct data_object *data_object = data_object_from_ID3DX10DataLoader(iface); > + DWORD ret; > + > + ok(InterlockedDecrement(&data_loader_load_count) >= 0, "unexpected call\n"); > + > + if (io_tid) > + io_tid = GetCurrentThreadId(); Was the condition meant to be reversed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2445

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/tests/d3dx10.c: > +static DWORD io_tid; > + > +struct data_object > +{ > + ID3DX10DataLoader ID3DX10DataLoader_iface; > + ID3DX10DataProcessor ID3DX10DataProcessor_iface; > + > + HANDLE load_started; > + HANDLE load_done; > + HANDLE decompress_done; > + HRESULT load_ret; > + > + DWORD process_tid; > +}; > + > +static struct data_object* data_object_from_ID3DX10DataLoader(ID3DX10DataLoader *iface) Nitpick, '*' placement. Please give a look at the ok() messages introduced in this patch, a number of them can be improved so that they're more consistent with the usual style "Got unexpected %#x.\n" or similar. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2444

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > + if (!InterlockedDecrement(&thread_pump->processing_count)) > + RtlWakeAddressAll((void *)&thread_pump->processing_count); > + } > + > + if (!(v = thread_pump->processing_count)) > + break; > + > + RtlWaitOnAddress((void *)&thread_pump->processing_count, &v, sizeof(v), NULL); > + } > + > + AcquireSRWLockExclusive(&thread_pump->device_lock); > + list_move_tail(&list, &thread_pump->device_queue); > + thread_pump->device_count = 0; > + ReleaseSRWLockExclusive(&thread_pump->device_lock); > + > + while(!list_empty(&list)) Nitpick, whitespace. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2443

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > list_add_tail(&thread_pump->device_queue, &work_item->entry); > thread_pump->device_count++; > InterlockedDecrement(&thread_pump->processing_count); > + RtlWakeAddressAll((void *)&thread_pump->processing_count); Why isn't this one conditional like the other cases? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2442

June 21, 2022

Re: [PATCH v3 0/9] MR272: d3dx10: Add D3DX10CreateThreadPump implementation.

by Matteo Bruni (＠Mystral)

Matteo Bruni (@Mystral) commented about dlls/d3dx10_43/d3dx10_43_main.c: > { > - FIXME("iface %p stub!\n", iface); > - return E_NOTIMPL; > + struct thread_pump *thread_pump = impl_from_ID3DX10ThreadPump(iface); > + HRESULT hr; > + LONG v; > + > + TRACE("iface %p.\n", iface); > + > + while(1) > + { > + if (FAILED((hr = ID3DX10ThreadPump_ProcessDeviceWorkItems(iface, UINT_MAX)))) > + return hr; > + > + if (!(v = thread_pump->processing_count)) > + break; I think there is a theoretical race here with a work item that's being added at the same time. Assuming that's a legitimate use case. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/272#note_2441

June 21, 2022