Skip to content
Snippets Groups Projects

tests: Add some tests for effects groups syntax.

Merged Nikolay Sivov requested to merge nsivov/vkd3d:fx2 into master

Signed-off-by: Nikolay Sivov nsivov@codeweavers.com

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
    • 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?]

    • Author Developer

      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.

    • Author Developer

      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.

    • Author Developer

      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.

    • 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.

    • Author Developer

      Ok, I pushed something that doesn't look too bad I think.

    • Please register or sign in to reply
  • Nikolay Sivov added 6 commits

    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.

    Compare with previous version

  • Giovanni Mascellani
  • Giovanni Mascellani
  • As I said I don't know much about effects (thanks for the pointer, anyway), so I just added a couple of general comments.

  • Nikolay Sivov added 1 commit

    added 1 commit

    • 3c1a8c6a - vkd3d-shader/tpf: Initial support for writing fx_4_0/fx_4_1 binaries.

    Compare with previous version

    • Do we want effects targets to have a dedicated target type?

    • Author Developer

      What do you mean?

    • I mean, should they really use VKD3D_SHADER_TARGET_DXBC_TPF / VKD3D_SHADER_TARGET_D3D_BYTECODE, or do we want a new target type? This is an open API design question.

    • Author Developer

      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.

    • Author Developer

      I pushed something to use separate target types for those. Please take a look.

    • I don't know much about the other effect versions, but superficially write_fx10() and write_sm4_shdr() look different enough to justify a different type.

      For the other types, I'd wait to expose them in the public header until we're able to emit at least something.

    • Please register or sign in to reply
  • Nikolay Sivov added 28 commits

    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.

    Compare with previous version

  • Nikolay Sivov added 7 commits

    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.

    Compare with previous version

  • Author Developer

    Pushed another iteration, with a single effect target, and explicit dxbc wrapping for fx_4_x.

  • Nikolay Sivov added 7 commits

    added 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.

    Compare with previous version

  • Nikolay Sivov added 27 commits

    added 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.

    Compare with previous version

  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 2f47cc8c
  • 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);
    • Comment on lines +109 to +110

      The point of set_u32() is that you can set these offsets after writing them, instead of needing to do two passes over the same data.

    • Author Developer

      I think this could also be avoided. What I'll probably end up with is two buffers, one for structured part - (cbuffers, variables, etc), and another for unstructured part for strings, default values, and so one. I think shader blobs also go in there.

    • 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...

    • Author Developer

      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.

    • That works too, but with set_u32() you don't need to know sizes in advance either, you just go back and set them once you do know.

    • Nikolay Sivov changed this line in version 8 of the diff

      changed this line in version 8 of the diff

    • Please register or sign in to reply
  • Elizabeth Figura
  • Elizabeth Figura
  • Elizabeth Figura
  • Elizabeth Figura
  • From a HLSL perspective this is mostly good. The API parts may need more bikeshedding.

  • From a HLSL perspective this is mostly good. The API parts may need more bikeshedding.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading