Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Some fixes.

Merged Francisco Casas requested to merge fcasas/vkd3d:fixes into master

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

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
  • Author Developer

    Oh, I forgot rebasing over the new tests... I will do it in short.

  • Francisco Casas added 10 commits

    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.

    Compare with previous version

  • Francisco Casas added 6 commits

    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.

    Compare with previous version

    • All of the tests pass for me with the native compiler, which implies that the changes in patch 5/7 are not correct as-is (and, somewhat by extension, neither are the changes in 4/7.)

    • Author Developer

      (!)

      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.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 3c2b293c
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
    • 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.

    • Author Developer

      Ok, then I will change all other uses of dimx for allocating a register to reg_size in v2.

    • Please register or sign in to reply
  • Francisco Casas added 4 commits

    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.

    Compare with previous version

  • Author Developer

    whoops, sending again because I forgot to include a test in hlsl-invalid.shader_test for statics with both resource and numeric components.

  • Francisco Casas added 3 commits

    added 3 commits

    • fabfcae2 - vkd3d-shader/hlsl: Validate that statics don't contain both resources and numerics.
    • bfc4d0dc - vkd3d-shader/hlsl: Use the base type of the array elements in write_sm1_type().
    • 0e647322 - vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

    • From 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?

    • Author Developer

      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 to const 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 of hlsl_type_get_component_type() makes it a little more correct.

      OTOH if I went full const, and I were to add const to the return type of hlsl_type_get_component_type() (which in principle seems to be the correct thing to do), then I would have to add it to the struct 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 of const struct hlsl_type *, so we would have to change those signatures too, and probably in the struct hlsl_type * members in structs like struct 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.

    • Propagating const to more functions is probably desirable in general. The problem is that it may be a losing battle, given "bytecode_offset". I'm not sure that avoiding that would be an improvement, though...

    • Personally I like to have everything as const as possible, but (in general) not at the expense of casting away const.

    • In the spirit of e.g. strchr(), I don't mind the patch as-is. I'll defer to Henri's preference, though.

    • Please register or sign in to reply
  • Giovanni Mascellani
  • OTOH if I went full const, and I were to add const to the return type of hlsl_type_get_component_type() (which in principle seems to be the correct thing to do), then I would have to add it to the struct 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 of const struct hlsl_type *, so we would have to change those signatures too, and probably in the struct hlsl_type * members in structs like struct 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.

  • Author Developer

    Dropping 'const' patch. And renaming within_struct parameter to must_be_in_struct ...

  • Francisco Casas added 5 commits

    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.

    Compare with previous version

  • Author Developer

    Rebasing on top of master...

  • Francisco Casas added 15 commits

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

    Compare with previous version

  • Author Developer

    I just realized that I used a tab instead of spaces in a test, and added triple new line in hlsl.y. Apologies. So, fixing those too...

  • Francisco Casas added 3 commits

    added 3 commits

    • 6b278032 - vkd3d-shader/hlsl: Validate that statics don't contain both resources and numerics.
    • dc1d92c7 - vkd3d-shader/hlsl: Use the base type of the array elements in write_sm1_type().
    • 9a38ca66 - vkd3d-shader/hlsl: Use reg_size as component count when allocating a single register.

    Compare with previous version

  • Giovanni Mascellani approved this merge request

    approved this merge request

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