vkd3d-shader/hlsl: Some fixes.
A mix of a miscellaneous fixes:
- Fixes to failed asserts I have stumbled upon when implementing other features.
- Checks required for properly supporting object components.
- A couple of code improvements.
Merge request reports
Activity
added 10 commits
-
8e5e7371...b711b2d6 - 3 commits from branch
wine:master
- c9ba0456 - vkd3d-shader/hlsl: Properly free new store node memory if init_deref() fails.
- afec0016 - vkd3d-shader/hlsl: Make hlsl_type_get_component_type() type argument
- 39ed972c - vkd3d-shader/hlsl: Don't allocate object types as constant registers.
- cb991c22 - tests: Add SM5 requirement to tests with object components.
- de4f4210 - vkd3d-shader/hlsl: Validate that objects are not components of structs in shader models < 5.
- a8bfed04 - vkd3d-shader/hlsl: For arrays, return components' base type in sm1_base_type().
- 55c6b4d1 - vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register.
Toggle commit list-
8e5e7371...b711b2d6 - 3 commits from branch
added 6 commits
- aa7c4393 - vkd3d-shader/hlsl: Make hlsl_type_get_component_type() type argument const.
- bc040ad6 - vkd3d-shader/hlsl: Don't allocate object types as constant registers.
- ad7555bd - tests: Add SM5 requirement to tests with object components.
- 9d3418a9 - vkd3d-shader/hlsl: Validate that objects are not components of structs in shader models < 5.
- 3c2b293c - vkd3d-shader/hlsl: For arrays, return components' base type in sm1_base_type().
- 6393cd10 - vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register.
Toggle commit list(!)
I see. So I was mistaken. It seems that objects can actually be components of structs in SM < 5 if they are used as local variables, as in the tests.
Probably I got confused, because, for instance, this shader fails in ps_4_0:
struct apple { int2 aa; Texture2D bb; float cc; }; struct apple a; // Uniform variable of type that has object components! float4 main() : sv_target { return float4(0, 1, 2, 3); }
and the error message is a little more general than it should be:
error X3090: ps_4_0 does not allow textures or samplers to be members of compound types
I will check more in detail which storage classes are allowed to have object members and which don't.
165 165 166 166 static D3DXPARAMETER_TYPE sm1_base_type(const struct hlsl_type *type) 167 167 { 168 if (type->type == HLSL_CLASS_ARRAY) 169 return sm1_base_type(type->e.array.type); 170 changed this line in version 4 of the diff
In other places where allocate_register() is called, dimx is used instead of reg_size. I think this doesn't cause an error even when structs contain objects (which get the dimx from their format type), thanks to the split_array_copies and split_struct_copies passes, but it makes me wonder why not simply use reg_size in those cases too.
As described off-list, the answer is mostly "historical reasons". When reg_size was introduced (Wine commit 5f18f9f75ac4!) it counted whole registers, mostly because the very earliest register allocation code I wrote was geared towards sm1, and sm4 support was only added later. And non-numeric registers simply weren't considered at that point.
added 4 commits
- e9d0c3ce - vkd3d-shader/hlsl: Validate that extern structs don't contain objects SM < 5.
- 90b41a6a - vkd3d-shader/hlsl: Validate that statics don't contain both resources and numerics.
- 530b41c8 - vkd3d-shader/hlsl: Use the base type of the array elements in write_sm1_type().
- 2186bb45 - vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register.
Toggle commit listFrom patch 2/7:
-struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type, +struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, const struct hlsl_type *type, unsigned int index) { while (!type_is_single_component(type)) traverse_path_from_component_index(ctx, &type, &index); - return type; + return (struct hlsl_type *) type; }
This ends up casting away const, which seems undesirable. Is that really unavoidable?
Well, my memory of why I did this is fuzzy. I remember that I spent a lot of time adding
const
modifiers until I settled into this.I think that the problem went along these lines:
In C, a double pointer
type **
cannot be casted implicitly toconst type **
without a warning. e.g.:int main() { int a = 2; int *p = &a; int **pp = &p; const int **cpp = pp; }
(there is an interesting SO thread here: https://stackoverflow.com/questions/5055655/double-pointer-const-correctness-warnings-in-c)
So, adding the const as the argument of
hlsl_type_get_component_type()
was more of a requirement to avoid this.I could have settled on doing:
struct hlsl_type *hlsl_type_get_component_type(struct hlsl_ctx *ctx, struct hlsl_type *type, unsigned int index) { cont struct hlsl_type *ctype = type; while (!type_is_single_component(type)) traverse_path_from_component_index(ctx, &ctype, &index); return (struct hlsl_type *) ctype; }
but adding the
const
in the signature ofhlsl_type_get_component_type()
makes it a little more correct.OTOH if I went full
const
, and I were to addconst
to the return type ofhlsl_type_get_component_type()
(which in principle seems to be the correct thing to do), then I would have to add it to thestruct hlsl_type *
variables in the callers:[const] hlsl_type *type; type = hlsl_type_get_component_type(·);
But these same variables are passed to other functions, and we have a lot of functions that receive
struct hlsl_type *
instead ofconst struct hlsl_type *
, so we would have to change those signatures too, and probably in thestruct hlsl_type *
members in structs likestruct hlsl_ir_var
too.Now, I must confess that I don't recall what was the motivation for this specific patch, besides performing a little step towards const correctness. I don't remember if we discussed this many moons ago... I guess I could remove it from the series if it just adds noise.
- Resolved by Giovanni Mascellani
OTOH if I went full
const
, and I were to addconst
to the return type ofhlsl_type_get_component_type()
(which in principle seems to be the correct thing to do), then I would have to add it to thestruct hlsl_type *
variables in the callers:[const] hlsl_type *type; type = hlsl_type_get_component_type(·);
But these same variables are passed to other functions, and we have a lot of functions that receive
struct hlsl_type *
instead ofconst struct hlsl_type *
, so we would have to change those signatures too, and probably in thestruct hlsl_type *
members in structs likestruct hlsl_ir_var
too.We went through making a bunch of things const in wined3d some years ago now, and you pretty much have to start from the leaf functions and work your way up from there. You'll likely still end up with a number of cases where it's just not worth it.
added 5 commits
- 5582565c - vkd3d-shader/hlsl: Don't allocate object types as constant registers.
- 72c0aea0 - vkd3d-shader/hlsl: Validate that extern structs don't contain objects SM < 5.
- 5ced03ae - vkd3d-shader/hlsl: Validate that statics don't contain both resources and numerics.
- cfcf7ea2 - vkd3d-shader/hlsl: Use the base type of the array elements in write_sm1_type().
- 2909d2b2 - vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register.
Toggle commit listadded 15 commits
-
2909d2b2...c416627e - 9 commits from branch
wine:master
- 6bc66ab2 - vkd3d-shader/hlsl: Properly free new store node memory if init_deref() fails.
- 2461be5a - vkd3d-shader/hlsl: Don't allocate object types as constant registers.
- 5a612b31 - vkd3d-shader/hlsl: Validate that extern structs don't contain objects SM < 5.
- 52a32146 - vkd3d-shader/hlsl: Validate that statics don't contain both resources and numerics.
- 64ce4bff - vkd3d-shader/hlsl: Use the base type of the array elements in write_sm1_type().
- 745bb46a - vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register.
Toggle commit list-
2909d2b2...c416627e - 9 commits from branch