tests: Add some tests for effects groups syntax.
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com
Merge request reports
Activity
From patch 3:
+ if (!(technique->name = hlsl_strdup(ctx, name))) + { + vkd3d_free(technique); + return NULL; + } + + if (!(var = hlsl_new_var(ctx, name, ctx->builtin_types.Void, loc, NULL, 0, NULL))) + { + hlsl_free_technique(technique); + return NULL; + }
I'm inclined to think we should either make techniques variables, or make them non-variables, rather than doing both.
(Also, creating a variable but setting the type to "void" is probably going to result in some less-than-perfectly-clear error messages).
As a side note, I won't ask to change it, but patch 3/5 does two different things which I feel could have been split: it creates actual objects for techniques, and separately it implements effect group syntax.
From patch 5/5:
+ if (technique_type == TECHNIQUE9) + { + skip = ctx->profile->major_version > 2; + } + else if (technique_type == TECHNIQUE10) + { + if (ctx->profile->type == VKD3D_SHADER_TYPE_EFFECT && ctx->profile->major_version == 2) + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, + "The 'technique10' keyword is invalid for this profile."); + + skip = ctx->profile->major_version == 2; + } + else if (technique_type == TECHNIQUE11) + { + if (ctx->profile->type == VKD3D_SHADER_TYPE_EFFECT && ctx->profile->major_version == 2) + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, + "The 'technique11' keyword is invalid for this profile."); + + skip = ctx->profile->major_version == 4; + }
This may be a casualty of introducing code that isn't actually used yet (which is itself not great), but it seems surprising that we're going to emit an hlsl_error() and also bother setting the skip field, especially when we aren't doing the same for the 11 variant.
Also, would it maybe be more idiomatic to just store the technique level, and implement this logic when we actually emit techniques? [Are we going to need to store that information anyway?]
I'm inclined to think we should either make techniques variables, or make them non-variables, rather than doing both.
(Also, creating a variable but setting the type to "void" is probably going to result in some less-than-perfectly-clear error messages).
There are not really variables. There needs to be some way to have technique blocks at top level, then fxgroup block at top level, and techniques within groups. Next and final nesting level are passes that are contained in techniques. I don't see how this can nicely fit in the type system. That's why I used void.
Regarding skips, now I think that using lists here might be a better fit, so that I can do cleanup based on profile, before going further.
So, if not using variables, the only functionality needed here is to have something dummy in global scope that takes up the name, within groups, or within techniques you can't have variables, so it's easy to check names there against already added techniques/passes.
And yes, some splitting is necessary. I wanted some comments first to figure out general direction.
There are not really variables. There needs to be some way to have technique blocks at top level, then fxgroup block at top level, and techniques within groups. Next and final nesting level are passes that are contained in techniques. I don't see how this can nicely fit in the type system. That's why I used void.
I don't immediately see why it can't? You don't need to encode any of this in the type system. Any data that techniques have can just go in the hlsl_ir_var instead.
This goes for other effect-specific objects too, probably. At least some of those act (even) more like variables.
Void variables aren't bad, but slightly better for error reporting purposes would be to introduce a HLSL_TYPE_TECHNIQUE.
Alternatively we could just check for a name conflict with techniques in every place that we check for a name conflict with variables. I don't know offhand how many such places there are, so I don't know how ugly this would end up being.
Ok, I can do TYPE_TECHNIQUE with CLASS_OBJECT, same for groups and passes. And later for other state types. Yes, hlsl_ir_var for group/technique/pass will need something extra in it.
It seems more sustainable to have them as regular variables so we don't need to chase conflict checks. I still think it's better to have a separate list for them and not use extern_vars. First so we don't need to filter them out for regular profiles compilation, and second so that order they appear in is stable and predictable, it's critical that order is preserved for index access. Maybe single list is already working well for that, but I think keeping effect objects slightly separately from normal shader compilation is not a bad idea.
I still think it's better to have a separate list for them and not use extern_vars. First so we don't need to filter them out for regular profiles compilation, and second so that order they appear in is stable and predictable, it's critical that order is preserved for index access. Maybe single list is already working well for that, but I think keeping effect objects slightly separately from normal shader compilation is not a bad idea.
I don't have a preëxisting opinion about that one, at least. Note that we already have some sorting logic for extern_vars, but that's not to pass a judgement on whether we should add more.
The main impetus for its addition was to collect globals, entry point parameters, and the entry point return value, all in one place, but that obviously isn't going to matter for techniques, or other things that are only in the global scope.
Looking again, I think having them as regular variables is not great, because name is optional for techniques and passes. And having a name is a minimum you'd expect from a variable.
We can generate names if necessary. It also shouldn't be too difficult to handle nameless variables, but generating a name is probably easier.
added 6 commits
- 2d8a6fdb - tests: Add some tests for effects groups syntax.
- 3024b0d0 - vkd3d-shader/hlsl: Add 'fxgroup' token.
- 1b02abd8 - vkd3d-shader/hlsl: Rename rule for top-level techniques.
- 26100017 - vkd3d-shader/hlsl: Add variables for techniques.
- d57dfa9f - vkd3d-shader/hlsl: Handle effect group statement.
- 84b6e017 - vkd3d-shader/tpf: Initial support for writing fx_4_0/fx_4_1 binaries.
Toggle commit list- Resolved by Giovanni Mascellani
I've never used effects. Do you have some resource of what they look like and how they are used?
- Resolved by Nikolay Sivov
- Resolved by Nikolay Sivov
added 1 commit
- 3c1a8c6a - vkd3d-shader/tpf: Initial support for writing fx_4_0/fx_4_1 binaries.
I think we can use VKD3D_SHADER_TARGET_NONE, and VKD3D_SHADER_TARGET_D3D_ASM if we'll ever want to disassembly them.
I don't know why such flexibility was introduced, maybe to compile hlsl to spriv binary in a single step? By default it makes sense to me to use corresponding formats for all profiles and indicate that with TARGET_NONE for example.
added 28 commits
-
3c1a8c6a...a03e78bf - 21 commits from branch
wine:master
- 4859e8dc - vkd3d-shader: Add separate binary target types for effects.
- 312ad90c - tests: Add some tests for effects groups syntax.
- b995c9e6 - vkd3d-shader/hlsl: Add 'fxgroup' token.
- 1531d240 - vkd3d-shader/hlsl: Rename rule for top-level techniques.
- 8e914042 - vkd3d-shader/hlsl: Add variables for techniques.
- aeb70c90 - vkd3d-shader/hlsl: Handle effect group statement.
- d39a2c4d - vkd3d-shader/tpf: Initial support for writing fx_4_0/fx_4_1 binaries.
Toggle commit list-
3c1a8c6a...a03e78bf - 21 commits from branch
added 7 commits
- 2e5cac03 - tests: Add some tests for effects groups syntax.
- e55f8095 - vkd3d-shader/hlsl: Add 'fxgroup' token.
- 9adcda29 - vkd3d-shader/hlsl: Rename rule for top-level techniques.
- becdd658 - vkd3d-shader/hlsl: Add variables for techniques.
- eb931955 - vkd3d-shader/hlsl: Handle effect group statement.
- f0011b6b - vkd3d-shader: Add separate binary target type for effects.
- 61497120 - vkd3d-shader/tpf: Initial support for writing fx_4_0/fx_4_1 binaries.
Toggle commit listadded 7 commits
- 7a2420db - tests: Add some tests for effects groups syntax.
- 691b2b84 - vkd3d-shader/hlsl: Add 'fxgroup' token.
- acb6f228 - vkd3d-shader/hlsl: Rename rule for top-level techniques.
- 79eac89c - vkd3d-shader/hlsl: Add variables for techniques.
- d5067134 - vkd3d-shader/hlsl: Handle effect group statement.
- c0ab1156 - vkd3d-shader: Add separate binary target type for effects.
- 67a39b11 - vkd3d-shader/tpf: Initial support for writing fx_4_0/fx_4_1 binaries.
Toggle commit listadded 27 commits
-
67a39b11...852eefc0 - 20 commits from branch
wine:master
- 16f1327e - tests: Add some tests for effects groups syntax.
- f1be6da8 - vkd3d-shader/hlsl: Add 'fxgroup' token.
- 07101d78 - vkd3d-shader/hlsl: Rename rule for top-level techniques.
- d6ecc42d - vkd3d-shader/hlsl: Add variables for techniques.
- 3d160639 - vkd3d-shader/hlsl: Handle effect group statement.
- c15e1329 - vkd3d-shader: Add separate binary target type for effects.
- 2f47cc8c - vkd3d-shader/tpf: Initial support for writing fx_4_0/fx_4_1 binaries.
Toggle commit list-
67a39b11...852eefc0 - 20 commits from branch
- libs/vkd3d-shader/fx.c 0 → 100644
95 { 96 struct fx_context fx_ctx = { .raw_section_size = 4 }; 97 struct vkd3d_bytecode_buffer buffer = { 0 }; 98 99 fx_foreach_technique(ctx->globals, fx_technique_collect_stats, &fx_ctx, &buffer); 100 fx_ctx.raw_section_size = align(fx_ctx.raw_section_size, 4); 101 102 put_u32(&buffer, ctx->profile->minor_version == 0 ? 0xfeff1001 : 0xfeff1011); /* Version. */ 103 put_u32(&buffer, 0); /* Buffer count. */ 104 put_u32(&buffer, 0); /* Variable count. */ 105 put_u32(&buffer, 0); /* Object count. */ 106 put_u32(&buffer, 0); /* Pool buffer count. */ 107 put_u32(&buffer, 0); /* Pool variable count. */ 108 put_u32(&buffer, 0); /* Pool object count. */ 109 put_u32(&buffer, fx_ctx.technique_count); 110 put_u32(&buffer, fx_ctx.raw_section_size); I say this mostly because, in the current form, it's not obvious that we need to do anything that complex. I.e. we can write sections one at a time, recording counts and sizes as we do so, and then go back and write those counts and sizes in the header. Maybe further logic will complicate that approach, though...
Writing them separately makes it easier because I don't need to know total unstructured size in advance. It won't complicate anything, I don't think. Offsets are relative to the beginning of unstructured block, so writing both, collecting stats for the header, and then doing header+buffer 1+buffer 2, should be enough.
One additional feature that's easy to add later is deduplicate string data - on windows you won't get the same string twice, except for fx_2_0. I haven't checked in details how it's stored there. It should work fine without this storage optimization.
changed this line in version 8 of the diff
- Resolved by Nikolay Sivov
- Resolved by Nikolay Sivov
- Resolved by Nikolay Sivov
- Resolved by Nikolay Sivov