[PATCH vkd3d v2 2/5] vkd3d-shader: Track bytecode buffer size in bytes.

[Matteo Bruni]

On Thu, Jul 1, 2021 at 4:55 AM Zebediah Figura <zfigura at codeweavers.com> wrote:
>
> And change the way we handle alignment.
>
> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
> ---
> v2: use uint8_t; add align_buffer and get_buffer_size helpers; fix the uniform
> table size calculation
>
>  libs/vkd3d-shader/hlsl.h         |   4 +-
>  libs/vkd3d-shader/hlsl_codegen.c | 104 +++++++++++++++----------------
>  2 files changed, 51 insertions(+), 57 deletions(-)
>
> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> index e0045acf..54273ac6 100644
> --- a/libs/vkd3d-shader/hlsl.h
> +++ b/libs/vkd3d-shader/hlsl.h
> @@ -126,7 +126,7 @@ struct hlsl_type
>      } e;
>
>      unsigned int reg_size;
> -    unsigned int bytecode_offset;
> +    size_t bytecode_offset;
>  };
>
>  struct hlsl_semantic
> @@ -144,7 +144,7 @@ struct hlsl_struct_field
>      struct hlsl_semantic semantic;
>      unsigned int reg_offset;
>
> -    unsigned int name_bytecode_offset;
> +    size_t name_bytecode_offset;
>  };
>
>  struct hlsl_reg
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index 9afa590a..8e842bd1 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -1328,72 +1328,62 @@ static struct hlsl_reg hlsl_reg_from_deref(const struct hlsl_deref *deref, const
>  struct bytecode_buffer
>  {
>      struct hlsl_ctx *ctx;
> -    uint32_t *data;
> -    size_t count, size;
> +    uint8_t *data;
> +    size_t size, capacity;
>      int status;
>  };
>
> -/* Returns the token index. */
> -static unsigned int put_dword(struct bytecode_buffer *buffer, uint32_t value)
> +static size_t put_bytes(struct bytecode_buffer *buffer, const void *bytes, size_t size, size_t alignment)
>  {
> -    unsigned int index = buffer->count;
> +    size_t prev_size = buffer->size;
> +    size_t offset = align(prev_size, alignment);
>
>      if (buffer->status)
> -        return index;
> +        return offset;
>
> -    if (!hlsl_array_reserve(buffer->ctx, (void **)&buffer->data, &buffer->size,
> -            buffer->count + 1, sizeof(*buffer->data)))
> +    if (!hlsl_array_reserve(buffer->ctx, (void **)&buffer->data, &buffer->capacity, offset + size, 1))
>      {
>          buffer->status = VKD3D_ERROR_OUT_OF_MEMORY;
> -        return index;
> +        return offset;
>      }
> -    buffer->data[buffer->count++] = value;
> +    memset(buffer->data + prev_size, 0xab, offset - prev_size);
> +    memcpy(buffer->data + offset, bytes, size);
> +    buffer->size = offset + size;
> +    return offset;
> +}
>
> -    return index;
> +static size_t put_dword(struct bytecode_buffer *buffer, uint32_t value)
> +{
> +    return put_bytes(buffer, &value, sizeof(value), sizeof(value));
>  }
>
> -/* Returns the token index. */
> -static unsigned int put_float(struct bytecode_buffer *buffer, float value)
> +static size_t put_float(struct bytecode_buffer *buffer, float value)
>  {
> -    union
> -    {
> -        float f;
> -        uint32_t u;
> -    } u;
> -    u.f = value;
> -    return put_dword(buffer, u.u);
> +    return put_bytes(buffer, &value, sizeof(value), sizeof(value));
>  }
>
> -static void set_dword(struct bytecode_buffer *buffer, unsigned int index, uint32_t value)
> +static void set_dword(struct bytecode_buffer *buffer, size_t offset, uint32_t value)
>  {
>      if (buffer->status)
>          return;
>
> -    assert(index < buffer->count);
> -    buffer->data[index] = value;
> +    assert(offset + sizeof(value) <= buffer->size);
> +    memcpy(buffer->data + offset, &value, sizeof(value));
>  }
>
> -/* Returns the token index. */
> -static unsigned int put_string(struct bytecode_buffer *buffer, const char *str)
> +static size_t put_string(struct bytecode_buffer *buffer, const char *string)
>  {
> -    unsigned int index = buffer->count;
> -    size_t len = strlen(str) + 1;
> -    unsigned int token_count = (len + 3) / sizeof(*buffer->data);
> -
> -    if (buffer->status)
> -        return index;
> +    return put_bytes(buffer, string, strlen(string) + 1, 1);
> +}
>
> -    if (!hlsl_array_reserve(buffer->ctx, (void **)&buffer->data, &buffer->size,
> -            buffer->count + token_count, sizeof(*buffer->data)))
> -    {
> -        buffer->status = E_OUTOFMEMORY;
> -        return index;
> -    }
> +static size_t align_buffer(struct bytecode_buffer *buffer, uint32_t alignment)
> +{
> +    return put_bytes(buffer, NULL, 0, alignment);
> +}
>
> -    buffer->data[buffer->count + token_count - 1] = 0xabababab;
> -    memcpy(buffer->data + buffer->count, str, len);
> -    buffer->count += token_count;
> -    return index;
> +static size_t get_buffer_size(struct bytecode_buffer *buffer)
> +{
> +    return buffer->size;
>  }
>
>  static uint32_t sm1_version(enum vkd3d_shader_type type, unsigned int major, unsigned int minor)
> @@ -1509,9 +1499,10 @@ static unsigned int get_array_size(const struct hlsl_type *type)
>  static void write_sm1_type(struct bytecode_buffer *buffer, struct hlsl_type *type, unsigned int ctab_start)
>  {
>      const struct hlsl_type *array_type = get_array_type(type);
> -    unsigned int fields_offset = 0, field_count = 0;
>      unsigned int array_size = get_array_size(type);
>      struct hlsl_struct_field *field;
> +    unsigned int field_count = 0;
> +    size_t fields_offset = 0;
>
>      if (type->bytecode_offset)
>          return;
> @@ -1524,12 +1515,12 @@ static void write_sm1_type(struct bytecode_buffer *buffer, struct hlsl_type *typ
>              write_sm1_type(buffer, field->type, ctab_start);
>          }
>
> -        fields_offset = (buffer->count - ctab_start) * sizeof(*buffer->data);
> +        fields_offset = get_buffer_size(buffer) - ctab_start;

This isn't quite what I was expecting when I proposed the helper.
You access the current buffer offset whenever you want to reference it
afterwards, either in some range computation or to overwrite the
buffer in that location with the final value. In all cases (I
believe?) what you actually want is the aligned offset, i.e. the
location where you're going to start writing whatever is going to come
next in the buffer. Which is why, in principle, every access to the
current buffer offset needs to take the alignment (specifically of the
next thing that's going into the buffer, whenever it might make a
difference) into consideration.

Renaming align_buffer to get_buffer_offset() (and getting rid of
get_buffer_size()) would do the trick, except it seems a bit ugly to
have a get_ function modify the buffer. So it's probably better to
only align the returned offset there without touching the buffer. That
means though that you still need to explicitly align the buffer after
the very last thing you're going to write (i.e. the current use of
align_buffer()). Unless...

I'm not sure off the top of my head but it might be the case that the
alignment is a property of the buffer (e.g. always 4 for SM1 DXBC). In
that case it might make sense to store the alignment right in struct
bytecode_buffer instead and, probably, align after writing the data in
put_bytes() instead of before.