Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Properly check argument counts and offset dimensions in texture methods.

Merged Francisco Casas requested to merge fcasas/vkd3d:parse_methods into master
8 unresolved threads

The objective of this patch series is to converge to the correct argument count checks and offset parameter dimensions when parsing the Load, Sample, SampleLevel, Gather (and variants) methods, for the different texture types.

I made the following table to summarize the expected arguments and their dimensions:

resource_methods_dims

Getting these required some extensive trial-and-error, because:

  • Official documentation about the methods is spread in different pages and in a somewhat inconsistent manner.
  • Automatic vector truncation, scalar broadcasting, and type conversion, make shaders that pass inexact types compile.
  • clamp and status arguments (for tiled resources) are not present in fxc 9, and yet, they are part of ps_5_0 and vs_5_0.
  • Some methods (Load() in particular) require the mipmap level as part of the location parameter (except for Multi-Sampled textures), while other don't.

For implementing new methods I recommend passing an invalid parameter count to them in fxc 10, so that it lists available overloads. And to do this for each texture type.

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
  • Ghost User requested review from @fcasas

    requested review from @fcasas

    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/19/22 19:28, Francisco Casas wrote:
      > diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
      > index ca3506e3..4d74d3ae 100644
      > --- a/libs/vkd3d-shader/hlsl.y
      > +++ b/libs/vkd3d-shader/hlsl.y
      > @@ -2402,26 +2402,39 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
      >               && object_type->sampler_dim != HLSL_SAMPLER_DIM_CUBEARRAY)
      >       {
      >           const unsigned int sampler_dim = hlsl_sampler_dim_count(object_type->sampler_dim);
      > +        const unsigned int offset_dim = hlsl_offset_dim_count(object_type->sampler_dim);
      >           struct hlsl_ir_resource_load *load;
      > +        struct hlsl_ir_node *offset = NULL;
      >           struct hlsl_ir_node *coords;
      > +        bool multisampled;
      > +
      > +
      
      Double newline here.
      
      > +        multisampled = object_type->sampler_dim == HLSL_SAMPLER_DIM_2DMS
      > +                || object_type->sampler_dim == HLSL_SAMPLER_DIM_2DMSARRAY;
      >   
      > -        if (object_type->sampler_dim == HLSL_SAMPLER_DIM_2DMS
      > -                || object_type->sampler_dim == HLSL_SAMPLER_DIM_2DMSARRAY)
      > +        if (params->args_count < 1 + multisampled || params->args_count > 3 + multisampled)
      >           {
      > -            FIXME("'Load' method for multi-sample textures.\n");
      > +            hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
      > +                    "Wrong number of arguments to method 'Load': expected betweed %u and %u, but got %u.",
      
      Typo, "betweed".
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/19/22 19:28, Francisco Casas wrote:
      > +        if (params->args_count > 2 + !!offset_dim)
      > +            hlsl_fixme(ctx, loc, "Tiled resource clamp argument.");
      
      LOD clamp isn't a tiled-resource thing, right?
      
      > +        if (params->args_count > 3 + !!offset_dim)
      > +            hlsl_fixme(ctx, loc, "Tiled resource status argument.");
      > +
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/19/22 19:28, Francisco Casas wrote:
      > -        if (params->args_count != 3 && params->args_count != 4)
      > +        if (params->args_count < 2 || params->args_count > 4 + !!offset_dim)
      
      Shouldn't that be "< 3"?
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/19/22 19:28, Francisco Casas wrote:
      > +static inline unsigned int hlsl_offset_dim_count(enum hlsl_sampler_dim dim)
      
      As long as we don't actually need to use this outside of hlsl.y, I'd 
      mildly prefer to keep it there.
      
      > +{
      > +    switch (dim)
      > +    {
      > +        case HLSL_SAMPLER_DIM_1D:
      > +        case HLSL_SAMPLER_DIM_1DARRAY:
      > +            return 1;
      > +        case HLSL_SAMPLER_DIM_2D:
      > +        case HLSL_SAMPLER_DIM_2DMS:
      > +        case HLSL_SAMPLER_DIM_2DARRAY:
      > +        case HLSL_SAMPLER_DIM_2DMSARRAY:
      > +            return 2;
      > +        case HLSL_SAMPLER_DIM_3D:
      > +            return 3;
      > +        case HLSL_SAMPLER_DIM_CUBE:
      > +        case HLSL_SAMPLER_DIM_CUBEARRAY:
      > +            return 0; // Offset parameters not supported for these types.
      
      That's a C99 comment. I don't know if there's a reason to avoid those 
      except for aesthetics, but we have thus far.
      
      > @@ -2472,10 +2473,10 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
      >                   hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, sampler_dim), loc)))
      >               return false;
      >   
      > -        if (params->args_count == 3)
      > +        if (!!offset_dim && params->args_count == 3)
      
      The !! is mildly redundant here.
      _______________________________________________
      wine-gitlab mailing list -- wine-gitlab@winehq.org
      To unsubscribe send an email to wine-gitlab-leave@winehq.org
    • Francisco Casas replied on the mailing list:

      Hello,
      
      On 19-08-22 22:05, Zebediah Figura (she/her) wrote:
      > On 8/19/22 19:28, Francisco Casas wrote:
      >> +static inline unsigned int hlsl_offset_dim_count(enum 
      >> hlsl_sampler_dim dim)
      > 
      > As long as we don't actually need to use this outside of hlsl.y, I'd 
      > mildly prefer to keep it there.
      > 
      
      Okay. I wrote it in hlsl.h because of its similarity with 
      hlsl_sampler_dim_count(), but I think it can be moved to hlsl.y because 
      the offset node type's dimx should be enough to access this information 
      afterwards.
      
      >> +{
      >> +    switch (dim)
      >> +    {
      >> +        case HLSL_SAMPLER_DIM_1D:
      >> +        case HLSL_SAMPLER_DIM_1DARRAY:
      >> +            return 1;
      >> +        case HLSL_SAMPLER_DIM_2D:
      >> +        case HLSL_SAMPLER_DIM_2DMS:
      >> +        case HLSL_SAMPLER_DIM_2DARRAY:
      >> +        case HLSL_SAMPLER_DIM_2DMSARRAY:
      >> +            return 2;
      >> +        case HLSL_SAMPLER_DIM_3D:
      >> +            return 3;
      >> +        case HLSL_SAMPLER_DIM_CUBE:
      >> +        case HLSL_SAMPLER_DIM_CUBEARRAY:
      >> +            return 0; // Offset parameters not supported for these 
      >> types.
      > 
      > That's a C99 comment. I don't know if there's a reason to avoid those 
      > except for aesthetics, but we have thus far.
      > 
      
      Oh right, I will change it.
    • Zebediah Figura (she/her) replied on the mailing list:

      On 8/22/22 10:11, Francisco Casas wrote:
      > Hello,
      > 
      > On 19-08-22 22:05, Zebediah Figura (she/her) wrote:
      >> On 8/19/22 19:28, Francisco Casas wrote:
      >>> +static inline unsigned int hlsl_offset_dim_count(enum 
      >>> hlsl_sampler_dim dim)
      >>
      >> As long as we don't actually need to use this outside of hlsl.y, I'd 
      >> mildly prefer to keep it there.
      >>
      > 
      > Okay. I wrote it in hlsl.h because of its similarity with 
      > hlsl_sampler_dim_count(), but I think it can be moved to hlsl.y because 
      > the offset node type's dimx should be enough to access this information 
      > afterwards.
      
      I figured as much, but I'd like to avoid static inlines wherever 
      possible. They clutter up the header with more code (that affects all 
      CUs), it's harder to notice when there are no more users, there's more 
      separation between the definition and its uses.
    • Francisco Casas replied on the mailing list:

      
      
      On 19-08-22 22:05, Zebediah Figura (she/her) wrote:
      > On 8/19/22 19:28, Francisco Casas wrote:
      >> +        if (params->args_count > 2 + !!offset_dim)
      >> +            hlsl_fixme(ctx, loc, "Tiled resource clamp argument.");
      > 
      > LOD clamp isn't a tiled-resource thing, right?
      > 
      >> +        if (params->args_count > 3 + !!offset_dim)
      >> +            hlsl_fixme(ctx, loc, "Tiled resource status argument.");
      >> +
      > _______________________________________________
      > wine-gitlab mailing list -- wine-gitlab@winehq.org
      > To unsubscribe send an email to wine-gitlab-leave@winehq.org
      
      Hmm, sorry MSDN made it sound like they are [1]:
      
      \`\`\`
      The new HLSL syntax is allowed only on devices with tiled resources 
      support. Each relevant HLSL method for tiled resources in the following 
      table accepts either one (feedback) or two (clamp and feedback in this 
      order) additional optional parameters.
      \`\`\`
      
      But it seems that, while tiled-resources support is required, the 
      associated instructions can be used on both tiled and non-tiled 
      resources [2].
      
      I will change the message to just:
      
      \`\`\`
      hlsl_fixme(ctx, loc, "Sample() clamp parameter.");
      \`\`\`
      
      ---
      
      [1]
      https://docs.microsoft.com/en-us/windows/win32/direct3d11/hlsl-tiled-resources-exposure
      
      [2]
      https://microsoft.github.io/DirectX-Specs/d3d/archive/D3D11_3_FunctionalSpec.htm#5.9.4.5.5%20Shader%20Instructions 
    • Francisco Casas replied on the mailing list:

      On 19-08-22 22:05, Zebediah Figura (she/her) wrote:
      > On 8/19/22 19:28, Francisco Casas wrote:
      >> -        if (params->args_count != 3 && params->args_count != 4)
      >> +        if (params->args_count < 2 || params->args_count > 4 + 
      >> !!offset_dim)
      > 
      > Shouldn't that be "< 3"?
      > 
      
      !
      
      Yes, that is correct.
  • Francisco Casas added 8 commits

    added 8 commits

    • 3adda9c7 - vkd3d-shader/hlsl: Parse the SampleLevel method.
    • 69828e59 - vkd3d-shader/hlsl: Use proper dimensions on Sample method offset parameter.
    • 298fc010 - vkd3d-shader/hlsl: Use proper dimensions on gather methods offset parameter.
    • 5308c98f - vkd3d-shader/hlsl: Properly check argument count in Sample method.
    • 1cee8a0d - vkd3d-shader/hlsl: Use proper dimensions on SampleLevel method offset parameter.
    • ec7e9fff - vkd3d-shader/hlsl: Properly check argument count in SampleLevel method.
    • e91e6389 - vkd3d-shader/hlsl: Properly check argument count in gather methods.
    • ae534ca7 - vkd3d-shader/hlsl: Add offset parameter to 'Load' method.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 20 commits

    • ae534ca7...3d9baef3 - 12 commits from branch wine:master
    • d6f45b73 - vkd3d-shader/hlsl: Parse the SampleLevel method.
    • 652906ae - vkd3d-shader/hlsl: Use proper dimensions on Sample method offset parameter.
    • 562f647c - vkd3d-shader/hlsl: Use proper dimensions on gather methods offset parameter.
    • c4be4a4e - vkd3d-shader/hlsl: Properly check argument count in Sample method.
    • 15b19b15 - vkd3d-shader/hlsl: Use proper dimensions on SampleLevel method offset parameter.
    • 32e6f594 - vkd3d-shader/hlsl: Properly check argument count in SampleLevel method.
    • 60963227 - vkd3d-shader/hlsl: Properly check argument count in gather methods.
    • 8e07423b - vkd3d-shader/hlsl: Add offset parameter to 'Load' method.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading