vkd3d-shader/hlsl: Properly check argument counts and offset dimensions in texture methods.
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:
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
Activity
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.
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.
Toggle commit listadded 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.
Toggle commit list-
ae534ca7...3d9baef3 - 12 commits from branch