Skip to content
Snippets Groups Projects

vkd3d-shader/hlsl: Add field-level documentation to the most important structs in hlsl.h.

Closed Francisco Casas requested to merge fcasas/vkd3d:field_level_doc into master
13 unresolved threads

I believe this information could be very useful for newcomers to the HLSL compiler.

Documentation often needs to reference other parts of the code, so it can be a problem to update these references if the referenced code is modified. However, I think that documentation in the data definitions (as opposed to documentation inside the functions) is easier to maintain, since each data type is declared in a single place, to which one often keeps going back for reference, so it is less probable to forget to update a comment.

Also, it opens up the possibility of explaining what things are, instead of how they are used, in some cases, avoiding these references to other places in the code.

I can think on the following additional benefits:

  • Updates in these documentation comments, when the way a field is used changes, or when new fields are added, can help making future commits more readable.
  • Some IDEs display the comments above the field/struct declaration when the cursor is over an use of this field/struct.

A hopefully not deceiving sketch:

Hopefully not deceiving sketch.

Merge request reports

Closed by Francisco CasasFrancisco Casas 2 years ago (Nov 16, 2022 9:31pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
114 114 HLSL_ROW_MAJOR
115 115 };
116 116
117 /* An HLSL data type. */
117 118 struct hlsl_type
  • Comment on lines +117 to 118

    Here and in other places the comment is quite obvious and doesn't help a lot. I would just add a comment when it adds something that was not evident before.

  • Please register or sign in to reply
  • Giovanni Mascellani
  • 114 114 HLSL_ROW_MAJOR
    115 115 };
    116 116
    117 /* An HLSL data type. */
    117 118 struct hlsl_type
    118 119 {
    120 /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */
    119 121 struct list entry;
    • Comment on lines +120 to 121

      From the field declaration it's already quite obvious that we are dealing with a linked list of objects of type hlsl_type, so I wouldn't repeat that piece of information. The only useful thing this I think should be mentioned here is which struct list this field chains to, and everything else descends from that. So I would write something like "Chained with hlsl_ctx->types". Though I am not sure if "chained" is really the best possible word.

      The same mostly applies to all other struct list, struct rb_entry and struct rb_tree.

    • Agreed in general. I think the phrasing I'd use is "entry in hlsl_ctx->types", but I'm not picky.

    • Thinking again, I wouldn't dislike a phrasing that distinguishes the container entry from the item entry (typing makes it already clear for RB trees, but not for lists). So maybe something like "Container entry in hlsl_ctx->types" here and "Item entry in hlsl_type->entry" in hlsl_ctx. The "direction" of the containment relationship is usually not hard to infer from the context, but if you're reading a struct for the first time sparing some mind cycles can be helpful.

    • Please register or sign in to reply
  • Giovanni Mascellani
  • 115 115 };
    116 116
    117 /* An HLSL data type. */
    117 118 struct hlsl_type
    118 119 {
    120 /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */
    119 121 struct list entry;
    122 /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's
    123 * name is used as key. */
    120 124 struct rb_entry scope_entry;
    125 /* These fields indicate to which category of hlsl_types this hlsl_type belongs.
    126 * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type.
    127 * If type is numeric, base_type is used to indicate the type of its components.
    128 * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */
    121 129 enum hlsl_type_class type;
    122 130 enum hlsl_base_type base_type;
    • Comment on lines +125 to 130

      Here too the first line doesn't tell much that is not sort of obvious from the field declaration, and the others, while certainly useful, are a bit messy. I would avoid writing any comment for type and write something like this for base_type:

      If type is HLSL_CLASS_SCALAR, HLSL_CLASS_VECTOR or HLSL_CLASS_MATRIX, then base_type is one of HLSL_TYPE_FLOAT, HLSL_TYPE_HALF, HLSL_TYPE_DOUBLE, HLSL_TYPE_INT, HLSL_TYPE_UINT and HLSL_TYPE_BOOL. If type is HLSL_CLASS_OBJECT, then base_type is one of HLSL_TYPE_SAMPLER, HLSL_TYPE_TEXTURE, HLSL_TYPE_UAV, HLSL_TYPE_PIXELSHADER, HLSL_TYPE_VERTEXSHADER, HLSL_TYPE_STRING and HLSL_TYPE_VOID. In all the other cases base_type is not used.

      Not directly related to your patches, but field declarations could be fixed in the first place to make them more consistent. Right now we have an hlsl_type struct with fields type (of type enum hlsl_type_class with enum values beginning with HLSL_CLASS_) and base_type (of type enum hlsl_base_type with enum values beginning with HLSL_TYPE_). I would rename to kind (of type enum hlsl_kind with enum values beginning with HLSL_KIND_) and base_type (of type enum hlsl_base_type with enum values beginning with HLSL_BASE_TYPE_). But that's for another patch set.

    • Here too the first line doesn't tell much that is not sort of obvious from the field declaration, and the others, while certainly useful, are a bit messy. I would avoid writing any comment for type and write something like this for base_type:

      If type is HLSL_CLASS_SCALAR, HLSL_CLASS_VECTOR or HLSL_CLASS_MATRIX, then base_type is one of HLSL_TYPE_FLOAT, HLSL_TYPE_HALF, HLSL_TYPE_DOUBLE, HLSL_TYPE_INT, HLSL_TYPE_UINT and HLSL_TYPE_BOOL. If type is HLSL_CLASS_OBJECT, then base_type is one of HLSL_TYPE_SAMPLER, HLSL_TYPE_TEXTURE, HLSL_TYPE_UAV, HLSL_TYPE_PIXELSHADER, HLSL_TYPE_VERTEXSHADER, HLSL_TYPE_STRING and HLSL_TYPE_VOID. In all the other cases base_type is not used.

      Maybe, although I think this is not exactly easier to read.

      Here's also where the messiness of hlsl_type kind of shows up. So, time to start a discussion: should we split up enum hlsl_base_type into numeric and object types, and use the existing union to distinguish the two? I.e. something like

      struct hlsl_type
      {
          struct list entry;
          struct rb_entry scope_entry;
          enum hlsl_type_class type;
          const char *name;
          unsigned int modifiers;
          union
          {
              struct
              {
                  enum hlsl_base_type base_type;
                  unsigned int dimx, dimy;
              } numeric;
              struct
              {
                  struct hlsl_struct_field *fields;
                  size_t field_count;
              } record;
              struct
              {
                  struct hlsl_type *type;
                  unsigned int elements_count;
              } array;
              struct
              {
                  enum hlsl_object_type object_type;
                  enum hlsl_sampler_dim sampler_dim;
                  struct hlsl_type *resource_format;
              } object;
          } e;
      };

      (I've moved a few more fields into the union while I'm at it.)

      The advantages of this, as I see it, are (a) self-documentation clarity, and (b) it lets us use fewer 'default' cases in things like dump_ir_constant(). [I like to avoid 'default' cases in general since they make it harder to notice when you've added a new enum value.] The disadvantage, of course, is churn. I don't think it'd make the code much more complex, though—the only place that really suffers is sm{1,4}_base_type(), but I'm okay with that.

      Not directly related to your patches, but field declarations could be fixed in the first place to make them more consistent. Right now we have an hlsl_type struct with fields type (of type enum hlsl_type_class with enum values beginning with HLSL_CLASS_) and base_type (of type enum hlsl_base_type with enum values beginning with HLSL_TYPE_). I would rename to kind (of type enum hlsl_kind with enum values beginning with HLSL_KIND_) and base_type (of type enum hlsl_base_type with enum values beginning with HLSL_BASE_TYPE_). But that's for another patch set.

      Despite the churn I'm inclined to agree with this. Although I don't see a need to rename "class" to "kind"; this isn't C++ after all ;-)

    • I'm totally in favor of Zeb's suggestion for struct hlsl_type. Only thing, I would use Struct instead of record (similarly to what we already to for hlsl_ctx::builtin_types::Void, but I can live with record too.

      As for "class" vs "kind", I tend to prefer "kind" anyway because in type theory it is used to denote something along the lines of "a type's type". But here too I can live with "class".

    • Please register or sign in to reply
  • Giovanni Mascellani
  • 120 /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */
    119 121 struct list entry;
    122 /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's
    123 * name is used as key. */
    120 124 struct rb_entry scope_entry;
    125 /* These fields indicate to which category of hlsl_types this hlsl_type belongs.
    126 * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type.
    127 * If type is numeric, base_type is used to indicate the type of its components.
    128 * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */
    121 129 enum hlsl_type_class type;
    122 130 enum hlsl_base_type base_type;
    131 /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or
    132 * sampler. */
    123 133 enum hlsl_sampler_dim sampler_dim;
    134 /* String buffer with the name of the hlsl_type. */
    124 135 const char *name;
  • Giovanni Mascellani
  • 125 137 unsigned int modifiers;
    138 /* Size of the type values on each dimension; primarily intended for numeric types: scalars,
    139 * vectors, and matrices.Other types may assign values to them for convenience.
    140 * See type initialization functions. */
    126 141 unsigned int dimx;
    127 142 unsigned int dimy;
    143
    128 144 union
    129 145 {
    146 /* Additional information if the type is HLSL_CLASS_STRUCT. */
    130 147 struct
    131 148 {
    149 /* Fields contained within the struct. */
    132 150 struct hlsl_struct_field *fields;
    151 /* Number of fields contained in the struct. */
    133 152 size_t field_count;
  • Giovanni Mascellani
  • 146 /* Additional information if the type is HLSL_CLASS_STRUCT. */
    130 147 struct
    131 148 {
    149 /* Fields contained within the struct. */
    132 150 struct hlsl_struct_field *fields;
    151 /* Number of fields contained in the struct. */
    133 152 size_t field_count;
    134 153 } record;
    154 /* Additional information if the type is HLSL_CLASS_ARRAY. */
    135 155 struct
    136 156 {
    157 /* Type of the array elements. */
    137 158 struct hlsl_type *type;
    159 /* Number of elements contained within the array or HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT
    160 * if it is unknown yet. */
    138 161 unsigned int elements_count;
  • Giovanni Mascellani
  • 117 118 struct hlsl_type
    118 119 {
    120 /* Linked list entry to store this hlsl_type in the context's linked list of hlsl_types. */
    119 121 struct list entry;
    122 /* Search tree entry to store this hlsl_type in the context's search tree of types. The type's
    123 * name is used as key. */
    120 124 struct rb_entry scope_entry;
    125 /* These fields indicate to which category of hlsl_types this hlsl_type belongs.
    126 * If type is HLSL_CLASS_OBJECT, base_type is used to indicate a subcategory of the type.
    127 * If type is numeric, base_type is used to indicate the type of its components.
    128 * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */
    121 129 enum hlsl_type_class type;
    122 130 enum hlsl_base_type base_type;
    131 /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or
    132 * sampler. */
    123 133 enum hlsl_sampler_dim sampler_dim;
    • Comment on lines +131 to 133

      In general I think that it's useful if comments like this spell out the precise rules, because there is good chance that that's what you'll look for if you read it. Something like:

      If base_type is HLSL_TYPE_SAMPLER then these are the allowed values: ...; if base_type is HLSL_TYPE_TEXTURE then these are the allowed values: ...; etc.

    • Please register or sign in to reply
  • Giovanni Mascellani
  • 127 * If type is numeric, base_type is used to indicate the type of its components.
    128 * base_type is not used when type is HLSL_CLASS_STRUCT or HLSL_CLASS_ARRAY. */
    121 129 enum hlsl_type_class type;
    122 130 enum hlsl_base_type base_type;
    131 /* Sampling dimension of the hlsl_type, in case it is a resource type, e.g. a texture or
    132 * sampler. */
    123 133 enum hlsl_sampler_dim sampler_dim;
    134 /* String buffer with the name of the hlsl_type. */
    124 135 const char *name;
    136 /* Bitfield for storing type modifiers, subset of HLSL_TYPE_MODIFIERS_MASK */
    125 137 unsigned int modifiers;
    138 /* Size of the type values on each dimension; primarily intended for numeric types: scalars,
    139 * vectors, and matrices.Other types may assign values to them for convenience.
    140 * See type initialization functions. */
    126 141 unsigned int dimx;
    127 142 unsigned int dimy;
    • Comment on lines +138 to 142

      Here too it is useful to know the precise rules for what dimx and dimy are set to depending on the base_type. Saying "See type initialization functions" kind of contradicts your intention of documenting data structures rather than code.

    • Currently I think we set dimx/dimy for non-numeric types basically for the benefit of the sm1/sm4 backends, which is, y'know, the kind of thing that should be documented here. That said, I'm not sure I like that fact (see also my above proposal, which would make that the job of the backend).

    • Please register or sign in to reply
  • In general I support the idea of documenting data structures rather than (or in addition to) code. But I think that your proposal still needs some work. I did a first pass on the first commit in order to outline what I think the philosophy of this kind of comments should be, so there can be some discussion.

  • Yeah, this patch is interesting. I'm not quite sure what to think of it. So some scattered thoughts below:

    • Fundamentally I think most (though not all) of these comments are obvious and unnecessary. But if they were that obvious, then we didn't need the comments in the first place, so I clearly need to adjust my expectations at least somewhat.

    • Looking at something like hlsl_type: intuitively I'd think it's pretty easy to guess what this is; it's a representation of a data type. It's also probably not hard to figure out what the contents of the type are just from the name, although some of them probably require an understanding of the HLSL language first.

      • More interesting are the questions that I think are not obvious just from looking at the source: is it source-level, or something further down? Is it unique? Where are all of the data types stored? [The answers, of course, being: yes, it's source-level; no, it's not unique; data types are stored in a list in hlsl_ctx.types].

      • There's also questions like: where are types constructed, and when are they retrieved? Same for individual fields—I can guess that the fields like base_type/dimx/dimy are set upon creation, but when is reg_size or bytecode_offset set and used?

      • Figuring out what kind of questions like this need to be asked is probably hard, though, and anticipating every question even harder. I'm basically trying to think of the kind of questions I've asked when I encounter a new project.

      • I think Giovanni kind of hit the nail on the head with one of the above comments—it's pretty obvious what sampler_dim means, the more interesting question is when is it set.

    • Then there's the awkward fact that a lot of the stuff that needs documentation should probably go away, like reg_size. (Coincidence?)

    • Some fields, like hlsl_ctx.profile, are not exactly clear just from the name and type what they are (because naming is hard and can only be so specific). But if you follow the chain to look at the contents of struct hlsl_profile_info, it becomes clearer. Is it still worth documenting hlsl_ctx.profile in that case? I don't know, maybe...

    • On the other hand, there's also stuff like hlsl_deref, which I think is a much less intuitive construction. I.e. I think it's clear what hlsl_type is, at least, but hlsl_deref is a bit more idiosyncratic. Same with hlsl_src (which is basically a node reference plus an entry in a def-use chain). So, y'know, not everything is obvious.

    I'll probably set this aside for a few days and keep thinking about it, unless of course we end up having more discussion in that time ;-)

    • Finding the right balance is never easy, particularly when targetting an audience with a potentially broad range of experience.

      Fundamentally though:

      • More so than more established projects like Wine, we'd like vkd3d (and vkd3d-shader in particular) to be easy to pick up for new/incidental contributors. If that implies spelling out some things that may be obvious to more experienced contributors, that seems fine to me. I.e., this shouldn't turn into a C tutorial, but if it ends up becoming something along the lines of "getting started with vkd3d-shader", that may not be a bad thing.

      • I think vkd3d in general could do with more/better documentation. I'm primarily concerned about user facing documentation like the public API reference, and higher level overviews for how to use vkd3d, vkd3d-shader, and vkd3d-utils in applications or libraries, but I can only encourage anyone willing to work on this.

      • I think it's fine to go through a couple of review rounds to see where improvements can be made before committing these, but I don't think it necessarily needs to be perfect before it can go in. We'd like to avoid comments that are misleading or just wrong, but the potential for regressions is pretty limited, and I'd expect this to evolve a bit after it's committed in any case.

    • I agree with these points.

      I would only suggest that this series be limited a bit from 14 patches, since otherwise reviewing all of them will take a lot longer.

    • Please register or sign in to reply
    • I thought I had replied again to this thread, but it seems by reply got lost somehow. Annoying.

      Anyway, if @fcasas is willing to keep steering this, I would suggest him to rework his proposal according to the criteria that were discussed. Even before, since also some type definition refactoring was suggested, maybe that could be an even earlier step (like refactoring struct hlsl_type, precomputing the name, renaming type and base_type together with their enums, something else?).

    • Maybe, but I'm also not sure it needs to block this patch. In line with what Henri said, there's no harm in having some of this documentation be partial or temporary.

    • Please register or sign in to reply
  • Author Developer

    Okay, I am writing v2 trying to follow the guidelines presented in your nice feedback.

    I will post my replies to some of your suggestions in separate comments so that we don't discuss everything in the same thread.

    • Author Developer

      Regarding obvious comments

      I will remove the obvious comments in the next version, but I think it is not a waste of time to explain why I wrote them in the first place, maybe there can be some interesting discussion.

      I understand that some comments are obvious, but a potential reader may not know if the absence of a comment means that the field should indeed be understood in the most obvious way, or it is not obvious at all and just missing documentation.

      Another reason why I added obvious comments is that, sadly, comments are not associated to a portion of the code (like, say, comments in a Google Docs document review), but instead inserted on it. So, sometimes an obvious comment is useful to indicate that a previous comment that explains multiple lines doesn't explain the following one, e.g. :

      /* Position of the reader */
      unsigned int page;
      unsigned int character;
      unsigned int frame_counter;

      versus

      /* Position of the reader */
      unsigned int page;
      unsigned int character;
      /* Counter of frames currently displayed */
      unsigned int frame_counter;

      In some personal projects I used to add comments with merely a v to indicate that the following line is self-explanatory, e.g.:

      /* v */
      unsigned int frame_counter;

      to address these problems, but I understand that that solution may be too unorthodox. Maybe empty comments are more acceptable, or at least adding empty lines to delimit the scope of the last comment:

      /* Position of the reader */
      unsigned int page;
      unsigned int character;
      
      unsigned int frame_counter;
    • Using empty lines to mark blocks of fields that are tied more strongly together is without doubt a good idea. As for obvious comments, I still have the feeling that something like

      /* Reader position */
      unsigned int reader_position;

      it much more likely to distract rather then help a reader, but I agree that's a rather extreme case. I will trust your judgement on there to draw the line.

    • Please register or sign in to reply
    • Author Developer

      Regarding the code changes suggested

      For convenience I will write a list here of the changes suggested:

      • Split enum hlsl_base_type into numeric and object types, and use the e union in struct hlsl_type to handle both cases separately. Making the dimx and dimy fields only for numeric types.
      • Rename enum hlsl_type_class to enum hlsl_type_kind.
      • Precompute hlsl_type_to_string() always.

      I agree with all these changes. However, the first one may require ubiquitous changes and complicate rebases, so I think we better properly coordinate for when we want to implement it. I would prefer to get most of the "broaden resources support" v2 patch series upstreamed first (I am still working on it), because it also modifies core data types, introducing ubiquious changes, and it may get complicated to upstream two of these changes at the same time.

      I agree in that these code changes shouldn't freeze documentation patches since, if we comment a piece of code that we will change later, the updates in the documentation will be additional information to make the commits more readable (provided we remember to do update it). This was the reason why I intented to send this patch series before "broaden resources support" v2.

    • Ok, it makes sense to concentrate on documentation now and leave code changes to later. Also because I am not sure they would be accepted, given that we are in code freeze.

    • Please register or sign in to reply
    • Author Developer

      Regarding general feedback for v2

      I think that most of the feedback goes in the direction of shifting from explaining what things are (which can often be extracted from the data type and name of the field) to actually explaining how they are used.

      Of course, these type of comments will be more coupled to the rest of the code, but I see now that they are more helpful (as long as we keep them updated).

      I can extract the following concerete things:

      • Explain the direction of containment relationships and directly refer to the field in the other type that is connected to it.
      • Explain where/when the fields/structs are initialized (unless it is obvious).
      • Explain where/when the fields are used (unless they are always used).
      • Explain how the type is stored (this can be done in the fields in charge of containment relationships).
      • Usually the last two are related to a level in the code: source, IR, or bytecode. They may also be related to specific shader models.
      • Note when a type is unique, i.e. instanced only once, like hlsl_ctx.
      • If a field has a non-trivial data type, assume that the reader can go to its definition and extract more information from it.
    • I would add: explain what formal rules an object of a type must satisfy. E.g., when this flag is set this field is relevant, otherwise it's not; or when this field is set to this value, then this other field must be non-negative. As usual, taking with a grain of salt if the explanation becomes so convoluted that it doesn't really help.

    • Please register or sign in to reply
  • I think I gave the impression in an earlier comment that "we shouldn't add comments for things that are self-explanatory", which either is not what I meant or is not how I feel now after thinking about it.

    I think there shouldn't be an expectation that we need to document every field just for the sake of documenting every field, but if you feel like a field would be improved with documentation, I don't think that there's any reason to bikeshed that. (Not least because, as the most recent person to work on the HLSL compiler, your thoughts should be given the heaviest weight.) Similarly explaining what a thing is doesn't necessarily hurt either. Especially for something like hlsl_deref which is kind of weird and non-obvious. (Similarly I could see the difference between hlsl_ir_function and hlsl_ir_function_decl being confusing.)

  • Francisco Casas mentioned in merge request !50 (merged)

    mentioned in merge request !50 (merged)

  • Author Developer

    Closing, since it is superseded by !50 (merged).

  • Please register or sign in to reply
    Loading