On Fri, Oct 29, 2021 at 9:01 PM Nikolay Sivov nsivov@codeweavers.com wrote:
On 10/29/21 9:35 PM, Matteo Bruni wrote:
On Thu, Oct 28, 2021 at 9:46 AM Nikolay Sivov nsivov@codeweavers.com wrote:
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com
dlls/d3d10/d3d10_private.h | 9 + dlls/d3d10/effect.c | 341 ++++++++++++++++++++++++++----------- dlls/d3d10/tests/effect.c | 207 ++++++++++++++++++---- 3 files changed, 420 insertions(+), 137 deletions(-)
diff --git a/dlls/d3d10/d3d10_private.h b/dlls/d3d10/d3d10_private.h index 11b3b4e9482..f8e415f860b 100644 --- a/dlls/d3d10/d3d10_private.h +++ b/dlls/d3d10/d3d10_private.h @@ -95,6 +95,13 @@ struct d3d10_effect_shader_variable unsigned int isinline : 1; };
+struct d3d10_effect_prop_dependencies +{
- struct d3d10_effect_prop_dependency *entries;
- SIZE_T count;
- SIZE_T capacity;
+};
struct d3d10_effect_sampler_desc { D3D10_SAMPLER_DESC desc; @@ -118,6 +125,7 @@ struct d3d10_effect_state_object_variable ID3D10SamplerState *sampler; IUnknown *object; } object;
- struct d3d10_effect_prop_dependencies dependencies;
};
struct d3d10_effect_resource_variable @@ -217,6 +225,7 @@ struct d3d10_effect_pass char *name; struct d3d10_effect_annotations annotations;
- struct d3d10_effect_prop_dependencies dependencies; struct d3d10_effect_pass_shader_desc vs; struct d3d10_effect_pass_shader_desc ps; struct d3d10_effect_pass_shader_desc gs;
diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c index 23374fdb48f..34a5eb1b701 100644 --- a/dlls/d3d10/effect.c +++ b/dlls/d3d10/effect.c @@ -168,6 +168,27 @@ enum d3d10_effect_container_type D3D10_C_SAMPLER, };
+struct d3d10_effect_prop_dependency +{
- unsigned int id;
- unsigned int idx;
- unsigned int operation;
- union
- {
struct{struct d3d10_effect_variable *v;unsigned int offset;} var;- } u;
+};
It should be possible to use anonymous unions now in d3d10, right? Not that you necessarily have to do that here, depending on what's going to happen to this struct in the following patches.
It's extended in 5/5. We don't currently use anonymous unions there, so I'm simply following existing pattern.
Right, I guess this seemed as good a time as any to start using them. Either way it's fine to me.
+static void d3d10_effect_update_dependent_props(struct d3d10_effect_prop_dependencies *deps,
void *container)+{
- const struct d3d10_effect_state_property_info *property_info;
- struct d3d10_effect_prop_dependency *d;
- struct d3d10_effect_variable *v;
- unsigned int i, j, count;
- uint32_t value;
- void *dst;
- for (i = 0; i < deps->count; ++i)
- {
d = &deps->entries[i];property_info = &property_infos[d->id];dst = (char *)container + property_info->offset;switch (d->operation){case D3D10_EOO_VAR:case D3D10_EOO_CONST_INDEX:v = d->u.var.v;count = v->type->type_class == D3D10_SVC_VECTOR ? 4 : 1;for (j = 0; j < count; ++j){d3d10_effect_variable_get_raw_value(v, &value, d->u.var.offset + j * sizeof(value), sizeof(value));d3d10_effect_read_numeric_value(value, v->type->basetype, property_info->type, dst, j);}break;default:FIXME("Unsupported property update for %u.\n", d->operation);}- }
+}
It would be nice to have dirty flags to avoid unnecessary recomputation. For example, something like the "changed" flag we have for buffers but referring to variables and their values. Problem is, there are a few quirks (e.g. SetRawValue(), those wild out-of-bounds accesses) that make this quite problematic. So I can be convinced to let it go :)
It would be useful for expressions I imagine, but even for expression it might be hard to quantify if all an expression does is a single ftou(), which is apparently common.
For value and index updates, I haven't measured obviously, because I didn't implement it.
The whole picture is:
foreach() { get_value(); put_value(); } use updated fields
which with change tracking will extend to
foreach() { if (changed) { get_value(); put_value(); replace_change_marker(); } } use updated fields
So when nothing changes it will still need to compare for each entry, and get/put is reduced to 4 byte read/write most of the time I think, if not always. Because e.g. setting things like arrays like blend factor produces complex expression, even if you set one element.
Regarding SetRawValue(), yes, the way it works is unfortunate, basically you'll have to track variables in constant buffer objects, so when SetRawValue is called on a buffer you mark them all as changed. But then when you do a larger than necessary raw value write on a variable, you potentially change adjacent variables too. Easy way is to mark all as changed when writing to buffers, and this and following (by offset) when writing to variables.
So that's additional overhead necessary to make property updates respect changed/dirty states.
Yeah, sounds like a lot of work for some pretty dubious benefit.
It would be something for a separate patch anyway.
@@ -1695,6 +1793,14 @@ static BOOL read_value_list(const char *data, size_t data_size, DWORD offset, *(void **)out_data = &null_shader_resource_variable; break;
case D3D10_SVT_DEPTHSTENCIL:*(void **)out_data = &null_depth_stencil_variable;break;case D3D10_SVT_BLEND:*(void **)out_data = &null_blend_variable;break;default: FIXME("Unhandled out_type %#x.\n", out_type); return FALSE;Would this also make sense as a separate patch?
It depends on how you look at it, e.g. setting stencil ref int with NULL object produces records like this, so it's sort of related. By I can split a much as necessary of course.
Yeah, I think it can stand on its own but it's not too ugly to have it in here.
@@ -1745,21 +1851,31 @@ static BOOL is_object_property_type_matching(const struct d3d10_effect_state_pro } }
+static HRESULT d3d10_effect_add_prop_dependency(struct d3d10_effect_prop_dependencies *d,
const struct d3d10_effect_prop_dependency *dep)+{
- if (!d3d_array_reserve((void **)&d->entries, &d->capacity, d->count + 1, sizeof(*d->entries)))
return E_OUTOFMEMORY;- d->entries[d->count++] = *dep;
- return S_OK;
+}
We probably want to go for the usual exponential growth pattern here. This separate helper is nice in that it can hide that kind of details away.
d3d_array_reserve() already does something like that, conditionally doubling sizes. You mean it's not enough?
No you're right, somehow I misread that.