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 (closed), trying to follow the wide feedback provided.
Following patches in: https://gitlab.winehq.org/fcasas/vkd3d/-/tree/documentation
Merge request reports
Activity
mentioned in merge request !38 (closed)
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. */ changed this line in version 2 of the diff
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; 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?")
-
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 forHLSL_STORAGE_NOINTERPOLATION
in thefield:
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.
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 theHLSL_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?
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.
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. */ changed this line in version 2 of the diff
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. */ changed this line in version 2 of the diff
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. changed this line in version 2 of the diff
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'. 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. */ changed this line in version 2 of the diff
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; 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. */ changed this line in version 2 of the diff
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; 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.]
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; 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.]
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.
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.
Toggle commit list- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani