Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Add field-level documentation to the most important structs in hlsl.h. (v2) (PART 1/2)

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

First part of v2 of !38 (closed), trying to follow the wide feedback provided.

Following patches in: https://gitlab.winehq.org/fcasas/vkd3d/-/tree/documentation

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
137 167 struct hlsl_type *type;
168 /* Array lenght, or HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT if it is unknown yet while parsing. */
138 169 unsigned int elements_count;
139 170 } array;
171 /* Format of the data contained within the type if the base_type is HLSL_TYPE_TEXTURE or
172 * HLSL_TYPE_UAV. */
140 173 struct hlsl_type *resource_format;
141 174 } e;
142 175
176 /* Number of numeric register components used by one value of this type (4 components make 1
177 * register).
178 * If type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY, this value includes the reg_size of
179 * their elements and padding (which varies according to the backend).
180 * This value is 0 for types without numeric components, like objects. */
143 181 unsigned int reg_size;
182 /* Offset where the type's description starts in the output bytecode. */
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit 9a58cff5
  • 192 193 struct hlsl_struct_field
    193 194 {
    194 195 struct vkd3d_shader_location loc;
    195 196 struct hlsl_type *type;
    196 197 const char *name;
    197 198 struct hlsl_semantic semantic;
    199
    200 /* Bitfield for storing type modifiers specific to this field, subset of
    201 * HLSL_TYPE_MODIFIERS_MASK, but can also include interpolation modifiers such as
    202 * HLSL_STORAGE_NOINTERPOLATION for pixel shader inputs. */
    198 203 unsigned int modifiers;
    204 /* Offset of the field within the type it belongs to, in numeric register components. */
    199 205 unsigned int reg_offset;
    200
    206 /* Offset where the fields's name starts in the output bytecode. */
    201 207 size_t name_bytecode_offset;
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 9a58cff5
  • 189 189 uint32_t index;
    190 190 };
    191 191
    192 /* A field within a struct type declaration, used in hlsl_type.e.fields. */
    192 193 struct hlsl_struct_field
    193 194 {
    194 195 struct vkd3d_shader_location loc;
    195 196 struct hlsl_type *type;
    196 197 const char *name;
    197 198 struct hlsl_semantic semantic;
    199
    200 /* Bitfield for storing type modifiers specific to this field, subset of
    201 * HLSL_TYPE_MODIFIERS_MASK, but can also include interpolation modifiers such as
    202 * HLSL_STORAGE_NOINTERPOLATION for pixel shader inputs. */
    • Comment on lines +200 to +202

      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?")

    • Author Developer

      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 :-)

    • Author Developer

      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.

      #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?

    • Francisco Casas changed this line in version 2 of the diff

      changed this line in version 2 of the diff

    • Eh. The oldest version of the code was a bit inconsistent. I changed some of them more recently to be more consistent, but I left VOLATILE alone. Currently it has no meaning even in the native compiler. Given that it can be put on a typedef, though, and otherwise behaves like "modifiers", I think it should be renamed to HLSL_MODIFIER_VOLATILE.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 8a8f00e3
  • 300 300 struct vkd3d_shader_location loc;
    301 301 const char *name;
    302 302 struct hlsl_semantic semantic;
    303 /* Buffer where the variable's value is stored, in case it is uniform. */
    303 304 struct hlsl_buffer *buffer;
    305 /* Bitfield for storage, matrix majority, and type modifiers. */
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 8a8f00e3
  • 300 300 struct vkd3d_shader_location loc;
    301 301 const char *name;
    302 302 struct hlsl_semantic semantic;
    303 /* Buffer where the variable's value is stored, in case it is uniform. */
    303 304 struct hlsl_buffer *buffer;
    305 /* Bitfield for storage, matrix majority, and type modifiers. */
    304 306 unsigned int modifiers;
    307 /* Optional register to be used as a starting point for the variable allocation. */
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit 8a8f00e3
  • 310 /* Item entry in hlsl_scope.vars. Specifically hlsl_ctx.globals.vars if the variable is global. */
    311 struct list scope_entry;
    312 /* Item entry in hlsl_ir_function_decl.parameters, if the variable is a function parameter. */
    313 struct list param_entry;
    314 /* Item entry in hlsl_ctx.extern_vars, if the variable is extern. */
    315 struct list extern_entry;
    316
    317 /* Indexes of the IR instructions where the variable is first written and last read (liveness
    318 * range). The IR instructions are numerated starting from 2, because 0 means unused, and 1
    319 * means function entry. */
    308 320 unsigned int first_write, last_read;
    321 /* Offset where the variable's value is stored within its buffer in numeric register components.
    322 * This in case the variable is uniform. */
    309 323 unsigned int buffer_offset;
    324 /* Register to which the variable is allocated during its lifetime.
    325 * In case that the variable uses more than one register, this refers to the starting one.
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit 8a8f00e3
  • 314 /* Item entry in hlsl_ctx.extern_vars, if the variable is extern. */
    315 struct list extern_entry;
    316
    317 /* Indexes of the IR instructions where the variable is first written and last read (liveness
    318 * range). The IR instructions are numerated starting from 2, because 0 means unused, and 1
    319 * means function entry. */
    308 320 unsigned int first_write, last_read;
    321 /* Offset where the variable's value is stored within its buffer in numeric register components.
    322 * This in case the variable is uniform. */
    309 323 unsigned int buffer_offset;
    324 /* Register to which the variable is allocated during its lifetime.
    325 * In case that the variable uses more than one register, this refers to the starting one.
    326 * The register type is inferred from the data type and the storage of the variable.
    327 * Uniforms and builtin semantics don't use the field.
    328 * If the variable is an input semantic copy, the register is 'v'.
    329 * If the variable is an output semantic copy, the register is 'o'.
    • Comment on lines +327 to +329

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

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit e3c161be
  • 566 566
    567 567 const char **source_files;
    568 568 unsigned int source_files_count;
    569 /* Current location being read in the HLSL source, updated while parsing. */
    569 570 struct vkd3d_shader_location location;
    571 /* Stores the logging messages and logging configuration. */
    570 572 struct vkd3d_shader_message_context *message_context;
    573 /* Stores string buffers currently in use. */
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit e3c161be
  • 566 566
    567 567 const char **source_files;
    568 568 unsigned int source_files_count;
    569 /* Current location being read in the HLSL source, updated while parsing. */
    569 570 struct vkd3d_shader_location location;
    571 /* Stores the logging messages and logging configuration. */
    570 572 struct vkd3d_shader_message_context *message_context;
    573 /* Stores string buffers currently in use. */
    571 574 struct vkd3d_string_buffer_cache string_buffers;
    575 /* A value from vkd3d_result. 0 means success. */
    572 576 int result;
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on an outdated change in commit e3c161be
  • 570 572 struct vkd3d_shader_message_context *message_context;
    573 /* Stores string buffers currently in use. */
    571 574 struct vkd3d_string_buffer_cache string_buffers;
    575 /* A value from vkd3d_result. 0 means success. */
    572 576 int result;
    573 577
    578 /* Pointer to an opaque data structure managed by FLEX (during lexing), that encapsulates the
    579 * current state of the scanner. This pointer is required by all FLEX API functions when the
    580 * scanner is declared as reentrant, which is the case. */
    574 581 void *scanner;
    575 582
    583 /* Pointer to the current scope; changes as the parser reads the code. */
    576 584 struct hlsl_scope *cur_scope;
    585 /* Scope of global variables. */
    577 586 struct hlsl_scope *globals;
    587 /* Container entry for hlsl_scope.entry fields, containing all the scopes in the program. */
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit e3c161be
  • 575 582
    583 /* Pointer to the current scope; changes as the parser reads the code. */
    576 584 struct hlsl_scope *cur_scope;
    585 /* Scope of global variables. */
    577 586 struct hlsl_scope *globals;
    587 /* Container entry for hlsl_scope.entry fields, containing all the scopes in the program. */
    578 588 struct list scopes;
    589 /* Container entry for hlsl_ir_var.extern_entry, containing all the extern variables. */
    579 590 struct list extern_vars;
    580 591
    592 /* Container entry for hlsl_buffer.entry, containing both the built-in HLSL buffers ($Globals
    593 * and $Params), and the ones declared in the shader. */
    581 594 struct list buffers;
    595 /* Current buffer (changes as the parser reads the code), $Globals buffer, and $Params buffer,
    596 * respectively. */
    582 597 struct hlsl_buffer *cur_buffer, *globals_buffer, *params_buffer;
    • Comment on lines +592 to 597

      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.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit e3c161be
  • 575 /* A value from vkd3d_result. 0 means success. */
    572 576 int result;
    573 577
    578 /* Pointer to an opaque data structure managed by FLEX (during lexing), that encapsulates the
    579 * current state of the scanner. This pointer is required by all FLEX API functions when the
    580 * scanner is declared as reentrant, which is the case. */
    574 581 void *scanner;
    575 582
    583 /* Pointer to the current scope; changes as the parser reads the code. */
    576 584 struct hlsl_scope *cur_scope;
    585 /* Scope of global variables. */
    577 586 struct hlsl_scope *globals;
    587 /* Container entry for hlsl_scope.entry fields, containing all the scopes in the program. */
    578 588 struct list scopes;
    589 /* Container entry for hlsl_ir_var.extern_entry, containing all the extern variables. */
    579 590 struct list extern_vars;
    • Comment on lines +589 to 590

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

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit 17cf6ac5
  • 228 233 HLSL_IR_SWIZZLE,
    229 234 };
    230 235
    236 /* Common data for every type of IR instruction node. */
    231 237 struct hlsl_ir_node
    232 238 {
    239 /* Item entry for storing the instruction in a list of instructions. Usually hlsl_block.instrs,
    240 * but also hlsl_ctx.static_initializers. */
    233 241 struct list entry;
    • Comment on lines +239 to 241

      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.

    • Please register or sign in to reply
  • Elizabeth Figura
    Elizabeth Figura @zfigura started a thread on commit 17cf6ac5
  • 228 233 HLSL_IR_SWIZZLE,
    229 234 };
    230 235
    236 /* Common data for every type of IR instruction node. */
    231 237 struct hlsl_ir_node
    232 238 {
    239 /* Item entry for storing the instruction in a list of instructions. Usually hlsl_block.instrs,
    240 * but also hlsl_ctx.static_initializers. */
    233 241 struct list entry;
    242
    243 /* Type of node, which means that a pointer to this struct hlsl_ir_node can be casted to a
    244 * pointer to the struct with the same name. */
    234 245 enum hlsl_ir_node_type type;
    246 /* HLSL data type of the node, when used as an expression. */
    235 247 struct hlsl_type *data_type;
    • Comment on lines +246 to 247

      "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.]

    • Please register or sign in to reply
  • 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.

  • Francisco Casas added 8 commits

    added 8 commits

    • b0eaf50f - vkd3d-shader/hlsl: Rename hlsl_ir_var.modifiers to "storage_modifiers".
    • a0288950 - vkd3d-shader/hlsl: Rename hlsl_struct_field.modifiers to "storage_modifiers".
    • 9beefe3c - vkd3d-shader/hlsl: Add field-level documentation to struct hlsl_type.
    • c867ea52 - vkd3d-shader/hlsl: Add field-level documentation to struct hlsl_struct_field.
    • 86e14188 - vkd3d-shader/hlsl: Add field-level documentation to struct hlsl_ir_var.
    • 0043a077 - vkd3d-shader/hlsl: Add field-level documentation to struct hlsl_ctx.
    • 53ca29d8 - vkd3d-shader/hlsl: Add field-level documentation to struct hlsl_ir_node.
    • a0b51a32 - vkd3d-shader/hlsl: Add field-level documentation to struct hlsl_src.

    Compare with previous version

  • Elizabeth Figura approved this merge request

    approved this merge request

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