[PATCH 0/6] MR50: vkd3d-shader/hlsl: Add field-level documentation to the most important structs in hlsl.h. (v2) (PART 1/2)
First part of v2 of !38, trying to follow the wide feedback provided. Following patches in: https://gitlab.winehq.org/fcasas/vkd3d/-/tree/documentation -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index b6a593ca..bf445655 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -114,33 +114,72 @@ enum hlsl_matrix_majority HLSL_ROW_MAJOR }; +/* An HLSL source-level data type, including anonymous structs and typedefs. */ struct hlsl_type { + /* Item entry in hlsl_ctx->types. */ struct list entry; + /* Item entry in hlsl_scope->types. hlsl_type->name is used as key (if not NULL). */ struct rb_entry scope_entry; + enum hlsl_type_class type; + /* If type is <= HLSL_CLASS_LAST_NUMERIC, then base_type is <= HLSL_TYPE_LAST_SCALAR. + * If type is HLSL_CLASS_OBJECT, then base_type is > HLSL_TYPE_LAST_SCALAR. + * Otherwise, base_type is not used. */ enum hlsl_base_type base_type; + + /* If base_type is HLSL_TYPE_SAMPLER, then sampler_dim is <= HLSL_SAMPLER_DIM_LAST_SAMPLER. + * If base_type is HLSL_TYPE_TEXTURE, then sampler_dim can have any value of the enum. + * If base_type is HLSL_TYPE_UAV, them sampler_dim must be one of HLSL_SAMPLER_DIM_1D, + * HLSL_SAMPLER_DIM_2D, HLSL_SAMPLER_DIM_3D, HLSL_SAMPLER_DIM_1DARRAY, or HLSL_SAMPLER_DIM_2DARRAY. + * Otherwise, sampler_dim is not used */ enum hlsl_sampler_dim sampler_dim; + /* Name, in case the type is a named struct or a typedef. */ const char *name; + /* Bitfield for storing type modifiers, subset of HLSL_TYPE_MODIFIERS_MASK. */ unsigned int modifiers; + /* Size of the type values on each dimension. For non-numeric types, they are set for the + * convenience of the sm1/sm4 backends. + * If type is HLSL_CLASS_SCALAR, then both dimx = 1 and dimy = 1. + * If type is HLSL_CLASS_VECTOR, then dimx is the size of the vector, and dimy = 1. + * If type is HLSL_CLASS_MATRIX, then dimx is the number of rows, and dimy the number of columns. + * If type is HLSL_CLASS_ARRAY, then dimx and dimy is the same as in the type of the array elements. + * If type is HLSL_CLASS_STRUCT, then dimx is the sum of (dimx * dimy) of every component, and dimy = 1. + * If type is HLSL_CLASS_OBJECT, dimx and dimy depend on the base_type: + * If base_type is HLSL_TYPE_SAMPLER, then both dimx = 1 and dimy = 1. + * If base_type is HLSL_TYPE_TEXTURE, then dimx = 4 and dimy = 1. + * If base_type is HLSL_TYPE_UAV, then dimx is the dimx of e.resource_format, and dimy = 1. + * Otherwise both dimx = 1 and dimy = 1. */ unsigned int dimx; unsigned int dimy; + union { + /* Additional information if type is HLSL_CLASS_STRUCT. */ struct { struct hlsl_struct_field *fields; size_t field_count; } record; + /* Additional information if type is HLSL_CLASS_ARRAY. */ struct { struct hlsl_type *type; + /* Array lenght, or HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT if it is unknown yet while parsing. */ unsigned int elements_count; } array; + /* Format of the data contained within the type if the base_type is HLSL_TYPE_TEXTURE or + * HLSL_TYPE_UAV. */ struct hlsl_type *resource_format; } e; + /* Number of numeric register components used by one value of this type (4 components make 1 + * register). + * If type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY, this value includes the reg_size of + * their elements and padding (which varies according to the backend). + * This value is 0 for types without numeric components, like objects. */ unsigned int reg_size; + /* Offset where the type's description starts in the output bytecode. */ size_t bytecode_offset; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index bf445655..0d1cdd93 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -189,15 +189,21 @@ struct hlsl_semantic uint32_t index; }; +/* A field within a struct type declaration, used in hlsl_type.e.fields. */ struct hlsl_struct_field { struct vkd3d_shader_location loc; struct hlsl_type *type; const char *name; struct hlsl_semantic semantic; + + /* Bitfield for storing type modifiers specific to this field, subset of + * HLSL_TYPE_MODIFIERS_MASK, but can also include interpolation modifiers such as + * HLSL_STORAGE_NOINTERPOLATION for pixel shader inputs. */ unsigned int modifiers; + /* Offset of the field within the type it belongs to, in numeric register components. */ unsigned int reg_offset; - + /* Offset where the fields's name starts in the output bytecode. */ size_t name_bytecode_offset; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 0d1cdd93..d1c91167 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -300,13 +300,36 @@ struct hlsl_ir_var struct vkd3d_shader_location loc; const char *name; struct hlsl_semantic semantic; + /* Buffer where the variable's value is stored, in case it is uniform. */ struct hlsl_buffer *buffer; + /* Bitfield for storage, matrix majority, and type modifiers. */ unsigned int modifiers; + /* Optional register to be used as a starting point for the variable allocation. */ struct hlsl_reg_reservation reg_reservation; - struct list scope_entry, param_entry, extern_entry; + /* Item entry in hlsl_scope.vars. Specifically hlsl_ctx.globals.vars if the variable is global. */ + struct list scope_entry; + /* Item entry in hlsl_ir_function_decl.parameters, if the variable is a function parameter. */ + struct list param_entry; + /* Item entry in hlsl_ctx.extern_vars, if the variable is extern. */ + struct list extern_entry; + + /* Indexes of the IR instructions where the variable is first written and last read (liveness + * range). The IR instructions are numerated starting from 2, because 0 means unused, and 1 + * means function entry. */ unsigned int first_write, last_read; + /* Offset where the variable's value is stored within its buffer in numeric register components. + * This in case the variable is uniform. */ unsigned int buffer_offset; + /* Register to which the variable is allocated during its lifetime. + * In case that the variable uses more than one register, this refers to the starting one. + * The register type is inferred from the data type and the storage of the variable. + * Uniforms and builtin semantics don't use the field. + * If the variable is an input semantic copy, the register is 'v'. + * If the variable is an output semantic copy, the register is 'o'. + * Textures are stored on 's' registers in SM1, and 't' registers in SM4. + * Samplers are stored on 's' registers. + * UAVs are stored on 'u' registers. */ struct hlsl_reg reg; uint32_t is_input_semantic : 1; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index d1c91167..b3bc4596 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -566,48 +566,79 @@ struct hlsl_ctx const char **source_files; unsigned int source_files_count; + /* Current location being read in the HLSL source, updated while parsing. */ struct vkd3d_shader_location location; + /* Stores the logging messages and logging configuration. */ struct vkd3d_shader_message_context *message_context; + /* Stores string buffers currently in use. */ struct vkd3d_string_buffer_cache string_buffers; + /* A value from vkd3d_result. 0 means success. */ int result; + /* Pointer to an opaque data structure managed by FLEX (during lexing), that encapsulates the + * current state of the scanner. This pointer is required by all FLEX API functions when the + * scanner is declared as reentrant, which is the case. */ void *scanner; + /* Pointer to the current scope; changes as the parser reads the code. */ struct hlsl_scope *cur_scope; + /* Scope of global variables. */ struct hlsl_scope *globals; + /* Container entry for hlsl_scope.entry fields, containing all the scopes in the program. */ struct list scopes; + /* Container entry for hlsl_ir_var.extern_entry, containing all the extern variables. */ struct list extern_vars; + /* Container entry for hlsl_buffer.entry, containing both the built-in HLSL buffers ($Globals + * and $Params), and the ones declared in the shader. */ struct list buffers; + /* Current buffer (changes as the parser reads the code), $Globals buffer, and $Params buffer, + * respectively. */ struct hlsl_buffer *cur_buffer, *globals_buffer, *params_buffer; + /* Container entry for hlsl_type.entry, containing all created hlsl_types, except builtin_types. */ struct list types; + /* Container for hlsl_ir_function.entry, containing the declared functions, using + * hlsl_ir_function.name as key. */ struct rb_tree functions; + /* Pointer to the current function; changes as the parser reads the code. */ const struct hlsl_ir_function_decl *cur_function; + /* Default matrix majority for matrix types. Can be set by a pragma within the HLSL source. */ enum hlsl_matrix_majority matrix_majority; + /* Basic data types stored for convenience. */ struct { struct hlsl_type *scalar[HLSL_TYPE_LAST_SCALAR + 1]; struct hlsl_type *vector[HLSL_TYPE_LAST_SCALAR + 1][4]; - /* matrix[float][2][4] is a float4x2, i.e. dimx = 2, dimy = 4 */ + /* matrix[HLSL_TYPE_FLOAT][1][3] is a float4x2, i.e. dimx = 2, dimy = 4 */ struct hlsl_type *matrix[HLSL_TYPE_LAST_SCALAR + 1][4][4]; struct hlsl_type *sampler[HLSL_SAMPLER_DIM_LAST_SAMPLER + 1]; struct hlsl_type *Void; } builtin_types; + /* Container entry for hlsl_ir_node.entry, containing the instruction nodes for initializing + * static variables. */ struct list static_initializers; + /* Dynamic array of constant values that appear in the shader, associated to the 'c' registers. + * Only used for SM1 profiles. */ struct hlsl_constant_defs { struct hlsl_vec4 *values; size_t count, size; } constant_defs; + /* Number of temp. registers required for the shader to run, i.e. the largest temp register + * index that will be used in the output bytecode (+1). */ uint32_t temp_count; + /* Number of threads to be executed (on the X, Y, and Z dimensions) in a single thread group in + * compute shader profiles. It is set using the numthreads() attribute in the entry point. */ uint32_t thread_count[3]; + /* Whether the parser is inside an state block (effects' metadata) inside a variable declarition. */ uint32_t in_state_block : 1; + /* Whether the numthreads() attribute has been provided in the entry-point function. */ uint32_t found_numthreads : 1; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index b3bc4596..1ab6ae4c 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -214,6 +214,11 @@ struct hlsl_reg bool allocated; }; +/* Types of instruction nodes for the IR. + * Each type of instruction node is associated to a struct with the same name in lower case. + * e.g. for HLSL_IR_CONSTANT there exists struct hlsl_ir_constant. + * Each one of these structs start with a struct hlsl_ir_node field, so pointers to values of these + * types can be casted seamlessly to (struct hlsl_ir_node *) and vice-versa. */ enum hlsl_ir_node_type { HLSL_IR_CONSTANT, @@ -228,12 +233,21 @@ enum hlsl_ir_node_type HLSL_IR_SWIZZLE, }; +/* Common data for every type of IR instruction node. */ struct hlsl_ir_node { + /* Item entry for storing the instruction in a list of instructions. Usually hlsl_block.instrs, + * but also hlsl_ctx.static_initializers. */ struct list entry; + + /* Type of node, which means that a pointer to this struct hlsl_ir_node can be casted to a + * pointer to the struct with the same name. */ enum hlsl_ir_node_type type; + /* HLSL data type of the node, when used as an expression. */ struct hlsl_type *data_type; + /* Container entry for hlsl_src.entry fields, containing all the struct hlsl_src·s that point + * to this node. */ struct list uses; struct vkd3d_shader_location loc; @@ -243,6 +257,7 @@ struct hlsl_ir_node * true even for loops, since currently we can't have a reference to a * value generated in an earlier iteration of the loop. */ unsigned int index, last_read; + /* Temp. register allocated to store the result of this instruction (if any). */ struct hlsl_reg reg; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 1ab6ae4c..56274742 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -266,9 +266,16 @@ struct hlsl_block struct list instrs; }; +/* A reference to an instruction node (struct hlsl_ir_node), usable as a field in other structs. + * struct hlsl_src is more powerful than a mere pointer to an hlsl_ir_node because it also contains + * a linked list item entry, which is used by the referenced instruction node to keep track of all + * the hlsl_src·s that reference it. + * This allows replacing any hlsl_ir_node with any other in all the places it is used, or checking + * that a node has no uses before it is removed. */ struct hlsl_src { struct hlsl_ir_node *node; + /* Item entry for node->uses. */ struct list entry; }; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
struct hlsl_type *type; + /* Array lenght, or HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT if it is unknown yet while parsing. */ unsigned int elements_count; } array; + /* Format of the data contained within the type if the base_type is HLSL_TYPE_TEXTURE or + * HLSL_TYPE_UAV. */ struct hlsl_type *resource_format; } e;
+ /* Number of numeric register components used by one value of this type (4 components make 1 + * register). + * If type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY, this value includes the reg_size of + * their elements and padding (which varies according to the backend). + * This value is 0 for types without numeric components, like objects. */ unsigned int reg_size; + /* Offset where the type's description starts in the output bytecode. */
Offset in bytes (right?) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17512
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
uint32_t index; };
+/* A field within a struct type declaration, used in hlsl_type.e.fields. */ struct hlsl_struct_field { struct vkd3d_shader_location loc; struct hlsl_type *type; const char *name; struct hlsl_semantic semantic; + + /* Bitfield for storing type modifiers specific to this field, subset of + * HLSL_TYPE_MODIFIERS_MASK, but can also include interpolation modifiers such as + * HLSL_STORAGE_NOINTERPOLATION for pixel shader inputs. */
This is wrong/misleading for two reasons: * HLSL_TYPE_MODIFIERS_MASK isn't actually stored here; it only goes on the type [v. apply_type_modifiers()]; * it's valid to put nointerpolation on other shader types. I also don't know if it's *correct* that we return an error for all other storage modifiers. But conceptually I wouldn't bother mentioning exactly which HLSL_STORAGE_* flags are valid because I don't think it's important in order to understand the code. I.e. any other storage flags are either going to be filtered out by hlsl_error() or they're just going to be ignored. [I do think it makes sense to mark that HLSL_MODIFIER_* is stored on the type but HLSL_STORAGE_* is on the hlsl_field/hlsl_var, lest someone ask "why do we have two different modifiers fields that store the same information?") -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17514
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
struct hlsl_struct_field { struct vkd3d_shader_location loc; struct hlsl_type *type; const char *name; struct hlsl_semantic semantic; + + /* Bitfield for storing type modifiers specific to this field, subset of + * HLSL_TYPE_MODIFIERS_MASK, but can also include interpolation modifiers such as + * HLSL_STORAGE_NOINTERPOLATION for pixel shader inputs. */ unsigned int modifiers; + /* Offset of the field within the type it belongs to, in numeric register components. */ unsigned int reg_offset; - + /* Offset where the fields's name starts in the output bytecode. */ size_t name_bytecode_offset; I'd leave the newline here. Also, output in bytes (right?)
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17513
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
struct vkd3d_shader_location loc; const char *name; struct hlsl_semantic semantic; + /* Buffer where the variable's value is stored, in case it is uniform. */ struct hlsl_buffer *buffer; + /* Bitfield for storage, matrix majority, and type modifiers. */
Here too, this should just be storage modifiers. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17515
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
struct vkd3d_shader_location loc; const char *name; struct hlsl_semantic semantic; + /* Buffer where the variable's value is stored, in case it is uniform. */ struct hlsl_buffer *buffer; + /* Bitfield for storage, matrix majority, and type modifiers. */ unsigned int modifiers; + /* Optional register to be used as a starting point for the variable allocation. */
Suggestion: "specified by the user via 'register(x0)' syntax"? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17516
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
+ /* Item entry in hlsl_ctx.extern_vars, if the variable is extern. */ + struct list extern_entry; + + /* Indexes of the IR instructions where the variable is first written and last read (liveness + * range). The IR instructions are numerated starting from 2, because 0 means unused, and 1 + * means function entry. */ unsigned int first_write, last_read; + /* Offset where the variable's value is stored within its buffer in numeric register components. + * This in case the variable is uniform. */ unsigned int buffer_offset; + /* Register to which the variable is allocated during its lifetime. + * In case that the variable uses more than one register, this refers to the starting one. + * The register type is inferred from the data type and the storage of the variable. + * Uniforms and builtin semantics don't use the field. + * If the variable is an input semantic copy, the register is 'v'. + * If the variable is an output semantic copy, the register is 'o'. Uniforms do use this field in sm1 [allocate_const_registers()], but not sm4. I'd mention also *why* it's unused for sm4 uniforms (viz. because they use the buffer hlsl_reg and buffer_offset instead.)
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17518
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
+ /* Item entry in hlsl_scope.vars. Specifically hlsl_ctx.globals.vars if the variable is global. */ + struct list scope_entry; + /* Item entry in hlsl_ir_function_decl.parameters, if the variable is a function parameter. */ + struct list param_entry; + /* Item entry in hlsl_ctx.extern_vars, if the variable is extern. */ + struct list extern_entry; + + /* Indexes of the IR instructions where the variable is first written and last read (liveness + * range). The IR instructions are numerated starting from 2, because 0 means unused, and 1 + * means function entry. */ unsigned int first_write, last_read; + /* Offset where the variable's value is stored within its buffer in numeric register components. + * This in case the variable is uniform. */ unsigned int buffer_offset; + /* Register to which the variable is allocated during its lifetime. + * In case that the variable uses more than one register, this refers to the starting one. This makes it sound like the variable can change registers. How about this wording:
"If the variable is larger than one register, this describes the start of the register range." -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17517
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
const char **source_files; unsigned int source_files_count; + /* Current location being read in the HLSL source, updated while parsing. */ struct vkd3d_shader_location location; + /* Stores the logging messages and logging configuration. */ struct vkd3d_shader_message_context *message_context; + /* Stores string buffers currently in use. */
This feels like it doesn't answer enough (and is kind of misleading, because buffers that are 'in use' at a given moment actually aren't in the struct). Maybe something like "cache for temporary string allocations"? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17519
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
struct vkd3d_shader_message_context *message_context; + /* Stores string buffers currently in use. */ struct vkd3d_string_buffer_cache string_buffers; + /* A value from vkd3d_result. 0 means success. */ int result;
+ /* Pointer to an opaque data structure managed by FLEX (during lexing), that encapsulates the + * current state of the scanner. This pointer is required by all FLEX API functions when the + * scanner is declared as reentrant, which is the case. */ void *scanner;
+ /* Pointer to the current scope; changes as the parser reads the code. */ struct hlsl_scope *cur_scope; + /* Scope of global variables. */ struct hlsl_scope *globals; + /* Container entry for hlsl_scope.entry fields, containing all the scopes in the program. */
"Container entry for" seems odd to me, also describing what the actual list contains last. I'd say "list of X, linked by Y.entry"? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17521
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
const char **source_files; unsigned int source_files_count; + /* Current location being read in the HLSL source, updated while parsing. */ struct vkd3d_shader_location location; + /* Stores the logging messages and logging configuration. */ struct vkd3d_shader_message_context *message_context; + /* Stores string buffers currently in use. */ struct vkd3d_string_buffer_cache string_buffers; + /* A value from vkd3d_result. 0 means success. */ int result;
A value from vkd3d_result that describes the success/failure of what, though, and is updated when? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17520
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
+ /* Pointer to the current scope; changes as the parser reads the code. */ struct hlsl_scope *cur_scope; + /* Scope of global variables. */ struct hlsl_scope *globals; + /* Container entry for hlsl_scope.entry fields, containing all the scopes in the program. */ struct list scopes; + /* Container entry for hlsl_ir_var.extern_entry, containing all the extern variables. */ struct list extern_vars;
+ /* Container entry for hlsl_buffer.entry, containing both the built-in HLSL buffers ($Globals + * and $Params), and the ones declared in the shader. */ struct list buffers; + /* Current buffer (changes as the parser reads the code), $Globals buffer, and $Params buffer, + * respectively. */ struct hlsl_buffer *cur_buffer, *globals_buffer, *params_buffer;
Not super important, but I notice that the wrapping here is inconsistent. I don't particularly mind the two-space indent either, but I think the usual style for comments we use is to never indent, and use a "blank" line between paragraphs. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17522
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
+ /* A value from vkd3d_result. 0 means success. */ int result;
+ /* Pointer to an opaque data structure managed by FLEX (during lexing), that encapsulates the + * current state of the scanner. This pointer is required by all FLEX API functions when the + * scanner is declared as reentrant, which is the case. */ void *scanner;
+ /* Pointer to the current scope; changes as the parser reads the code. */ struct hlsl_scope *cur_scope; + /* Scope of global variables. */ struct hlsl_scope *globals; + /* Container entry for hlsl_scope.entry fields, containing all the scopes in the program. */ struct list scopes; + /* Container entry for hlsl_ir_var.extern_entry, containing all the extern variables. */ struct list extern_vars; Why does this exist? [Answer: it's a convenience, to gather together extern variables which can be declared in global scope, as function parameters, or as the function return value. In many cases we need to iterate over all extern variables, and it's more convenient just to put them all in one list first.]
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17523
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
HLSL_IR_SWIZZLE, };
+/* Common data for every type of IR instruction node. */ struct hlsl_ir_node { + /* Item entry for storing the instruction in a list of instructions. Usually hlsl_block.instrs, + * but also hlsl_ctx.static_initializers. */ struct list entry;
Well, currently it's more than that, we pass around bare list pointers in quite a lot of places :-/ For now I'd just omit the latter sentence. Hopefully we can replace all of those with hlsl_block soon. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17524
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
HLSL_IR_SWIZZLE, };
+/* Common data for every type of IR instruction node. */ struct hlsl_ir_node { + /* Item entry for storing the instruction in a list of instructions. Usually hlsl_block.instrs, + * but also hlsl_ctx.static_initializers. */ struct list entry; + + /* Type of node, which means that a pointer to this struct hlsl_ir_node can be casted to a + * pointer to the struct with the same name. */ enum hlsl_ir_node_type type; + /* HLSL data type of the node, when used as an expression. */ struct hlsl_type *data_type;
"When used as an expression" is a bit odd wording given that what actually matters is the node type. I.e. CONSTANT, EXPR, LOAD, RESOURCE_LOAD, SWIZZLE have a data type and can be used in hlsl_src; everything else doesn't. [This seems salient to me because this wasn't always true. Once upon a time HLSL_IR_STORE was called HLSL_IR_ASSIGNMENT and it could return a value, basically representing statements like "a = (b = c);". Now we just copy the rhs.] -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17525
Sorry for taking so long to get to this. Most of these comments look fine, I've just nitpicked a few that seemed inaccurate or misleading. Also a few that I feel could use a bit more explanation, but I don't particularly want to block this patch just on the grounds of "we could use more documentation", so feel free to just ignore those comments for now. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17526
On Sat Nov 26 01:25:40 2022 +0000, Zebediah Figura wrote:
This makes it sound like the variable can change registers. How about this wording: "If the variable is larger than one register, this describes the start of the register range." Or "spans multiple registers", perhaps.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17658
HLSL_TYPE_MODIFIERS_MASK isn't actually stored here; it only goes on the type \[v. apply_type_modifiers()\];
Ah, I see now.
it's valid to put nointerpolation on other shader types.
Yep, I failed to mention that pixel shader inputs can also be vertex shader outputs.
I also don't know if it's *correct* that we return an error for all other storage modifiers. But conceptually I wouldn't bother mentioning exactly which HLSL_STORAGE\_\* flags are valid because I don't think it's important in order to understand the code. I.e. any other storage flags are either going to be filtered out by hlsl_error() or they're just going to be ignored.
We currently trigger an error for all modifiers that are not consumed by `apply_type_modifiers()` except for `HLSL_STORAGE_NOINTERPOLATION` in the `field:` rule. I will just mention that interpolation modifiers are a use case for the field.
\[I do think it makes sense to mark that HLSL_MODIFIER\_\* is stored on the type but HLSL_STORAGE\_\* is on the hlsl_field/hlsl_var, lest someone ask "why do we have two different modifiers fields that store the same information?")
Got it, I will see how to make this more clear. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17784
On Mon Nov 28 23:15:57 2022 +0000, Francisco Casas wrote:
HLSL_TYPE_MODIFIERS_MASK isn't actually stored here; it only goes on the type \[v. apply_type_modifiers()\]; Ah, I see now. it's valid to put nointerpolation on other shader types. Yep, I failed to mention that pixel shader inputs can also be vertex shader outputs. I also don't know if it's *correct* that we return an error for all other storage modifiers. But conceptually I wouldn't bother mentioning exactly which HLSL_STORAGE\_\* flags are valid because I don't think it's important in order to understand the code. I.e. any other storage flags are either going to be filtered out by hlsl_error() or they're just going to be ignored. We currently trigger an error for all modifiers that are not consumed by `apply_type_modifiers()` except for `HLSL_STORAGE_NOINTERPOLATION` in the `field:` rule. I will just mention that interpolation modifiers are a use case for the field. \[I do think it makes sense to mark that HLSL_MODIFIER\_\* is stored on the type but HLSL_STORAGE\_\* is on the hlsl_field/hlsl_var, lest someone ask "why do we have two different modifiers fields that store the same information?") Got it, I will see how to make this more clear. Honestly, "rename the field to 'storage_flags'" wouldn't be a bad idea.
Clearly, if nothing else, these documentation patches are pointing out a lot of things that we could do better in terms of code naming and organization. Which is probably not unexpected :-) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17785
On Tue Nov 29 00:01:43 2022 +0000, Zebediah Figura wrote:
Honestly, "rename the field to 'storage_flags'" wouldn't be a bad idea. Clearly, if nothing else, these documentation patches are pointing out a lot of things that we could do better in terms of code naming and organization. Which is probably not unexpected :-) Okay.
By the way I didn't thought that the separation between type modifiers and storage modifiers was that clear. In particular because `HLSL_STORAGE_VOLATILE` is in the `HLSL_TYPE_MODIFIERS_MASK`. ```c #define HLSL_TYPE_MODIFIERS_MASK (HLSL_MODIFIER_PRECISE | HLSL_STORAGE_VOLATILE | \ HLSL_MODIFIER_CONST | HLSL_MODIFIER_ROW_MAJOR | \ HLSL_MODIFIER_COLUMN_MAJOR) ``` Is this a mistake? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/50#note_17880
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura (＠zfigura)