From: Nikolay Sivov nsivov@codeweavers.com
--- libs/vkd3d-shader/hlsl.y | 45 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 209428f7..3f08b895 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -3709,6 +3709,7 @@ static unsigned int hlsl_offset_dim_count(enum hlsl_sampler_dim dim) return 3; case HLSL_SAMPLER_DIM_CUBE: case HLSL_SAMPLER_DIM_CUBEARRAY: + case HLSL_SAMPLER_DIM_BUFFER: /* Offset parameters not supported for these types. */ return 0; default: @@ -3728,6 +3729,38 @@ static bool raise_invalid_method_object_type(struct hlsl_ctx *ctx, const struct return false; }
+static bool add_buffer_load_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *object, + const char *name, const struct parse_initializer *params, const struct vkd3d_shader_location *loc) +{ + struct hlsl_resource_load_params load_params = {.type = HLSL_RESOURCE_LOAD}; + const struct hlsl_type *object_type = object->data_type; + struct hlsl_ir_node *load; + + if (params->args_count != 1 && params->args_count != 2) + { + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Wrong number of arguments to method 'Load': expected 1 or 2, but got %u.", params->args_count); + return false; + } + + if (params->args_count > 1) + { + hlsl_fixme(ctx, loc, "Tiled resource status argument."); + } + + if (!(load_params.coords = add_implicit_conversion(ctx, instrs, params->args[0], + hlsl_get_scalar_type(ctx, HLSL_TYPE_INT), loc))) + return false; + + load_params.format = object_type->e.resource_format; + load_params.resource = object; + + if (!(load = hlsl_new_resource_load(ctx, &load_params, loc))) + return false; + list_add_tail(instrs, &load->entry); + return true; +} + static bool add_load_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_node *object, const char *name, const struct parse_initializer *params, const struct vkd3d_shader_location *loc) { @@ -3738,6 +3771,12 @@ static bool add_load_method_call(struct hlsl_ctx *ctx, struct list *instrs, stru struct hlsl_ir_node *load; bool multisampled;
+ if (object_type->sampler_dim == HLSL_SAMPLER_DIM_BUFFER + || object_type->sampler_dim == HLSL_SAMPLER_DIM_STRUCTURED_BUFFER) + { + return add_buffer_load_method_call(ctx, instrs, object, name, params, loc); + } + if (object_type->sampler_dim == HLSL_SAMPLER_DIM_CUBE || object_type->sampler_dim == HLSL_SAMPLER_DIM_CUBEARRAY) { @@ -5093,7 +5132,11 @@ parameter: }
texture_type: - KW_TEXTURE1D + KW_BUFFER + { + $$ = HLSL_SAMPLER_DIM_BUFFER; + } + | KW_TEXTURE1D { $$ = HLSL_SAMPLER_DIM_1D; }
This merge request was approved by Zebediah Figura.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
struct hlsl_ir_node *load; bool multisampled;
- if (object_type->sampler_dim == HLSL_SAMPLER_DIM_BUFFER
|| object_type->sampler_dim == HLSL_SAMPLER_DIM_STRUCTURED_BUFFER)- {
return add_buffer_load_method_call(ctx, instrs, object, name, params, loc);- }
As I said somewhere else, I don't like this kind of control flow too much. Something in the form: ```c if (condition) { return f(); }
// do work ``` seems to suggest that `f()` is somehow an exceptional or simpler case of the main branch. Here that's not the case: the two branches are essentially equivalent. So I would rather move the main branch in a dedicated helper too and have something like: ```c if (condition) return f(); else return g(); ``` which suggests symmetry between the two alternatives.
Something I would like even more is to add a knowledge of the supported classes directly in `struct method_function`, but that's more complicated so I'm not necessarily pushing for that.
The code seems fine, except for a little detail. However, as usual, I'd like to have a few tests.
On Tue Jun 13 09:36:49 2023 +0000, Giovanni Mascellani wrote:
As I said somewhere else, I don't like this kind of control flow too much. Something in the form:
if (condition) { return f(); } // do workseems to suggest that `f()` is somehow an exceptional or simpler case of the main branch. Here that's not the case: the two branches are essentially equivalent. So I would rather move the main branch in a dedicated helper too and have something like:
if (condition) return f(); else return g();which suggests symmetry between the two alternatives. Something I would like even more is to add a knowledge of the supported classes directly in `struct method_function`, but that's more complicated so I'm not necessarily pushing for that.
To me readability is more important that a little bit of duplication here. New helper could be used for all buffer objects methods like Load2(), Load3(), Load4(), existing helper is mostly for textures.
Regarding if () {} else {} style, it's nothing but a matter a taste in my opinion. I don't care at all about that part, meaning I'll using whichever is preferred. I'd think more about it being consistent, if there is a broad agreement to use this style, maybe we could add formatting config file to make it clear.
Regarding if () {} else {} style, it's nothing but a matter a taste in my opinion. I don't care at all about that part, meaning I'll using whichever is preferred. I'd think more about it being consistent, if there is a broad agreement to use this style, maybe we could add formatting config file to make it clear.
To make the discussion more interesting, I'm generally on the opposite side of this. I.e., generally preferring
```c if (condition) return f(); return g(); ``` and particularly ```c if (condition1) return f(); if (condition2) return g(); return h(); ``` over the alternatives. A large part of that is simply the fact that "else" after "return" is redundant.
That said, while I'm happy to give my opinion on code style, I'm not a major contributor to the HLSL compiler at this point, and I've largely left it up to the people who are to figure out between themselves, provided it doesn't deviate too egregiously from the broader vkd3d style.
On Tue Jun 13 11:34:09 2023 +0000, Henri Verbeet wrote:
Regarding if () {} else {} style, it's nothing but a matter a taste in
my opinion. I don't care at all about that part, meaning I'll using whichever is preferred. I'd think more about it being consistent, if there is a broad agreement to use this style, maybe we could add formatting config file to make it clear. To make the discussion more interesting, I'm generally on the opposite side of this. I.e., generally preferring
if (condition) return f(); return g();and particularly
if (condition1) return f(); if (condition2) return g(); return h();over the alternatives. A large part of that is simply the fact that "else" after "return" is redundant. That said, while I'm happy to give my opinion on code style, I'm not a major contributor to the HLSL compiler at this point, and I've largely left it up to the people who are to figure out between themselves, provided it doesn't deviate too egregiously from the broader vkd3d style.
Of course I don't want this issue to become too predominant, given that it's just a matter of taste. I mentioned it mostly because at some point I thought my suggestions was somewhat aligned with Zeb's tastes too (but I could be wrong).
My point, though, is not really about the `else` (which I still prefer to have, if the two branches are somewhat symmetrical), but rather the fact of "hand-inlining" the default helper. I.e., I consider this a little suboptimal: ```c if (condition) return f(); return g(); ``` but I consider this significantly worse: ```c if (condition) return f(); // inline g here ``` (though, in the end, acceptable)
On Thu Jun 15 05:05:54 2023 +0000, Giovanni Mascellani wrote:
The code seems fine, except for a little detail. However, as usual, I'd like to have a few tests.
Regarding tests, this is confusing. I added a simple one that works on Windows, but with vkd3d I get compilation errors:
E2000: Could not find descriptor binding for type 0, space 0, registers [1:1], shader type 0.
Probably shader runner is somehow not prepared for Buffer type, and I thought it'd exactly the same as a texture.
-
Giovanni Mascellani (＠giomasce) -
Henri Verbeet (＠hverbeet)
-
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)