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

[Zebediah Figura (she/her)]

On 7/1/21 4:29 AM, Matteo Bruni wrote:
> 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.

That's essentially what we were doing before. The actual rule seems to 
be that alignment is always 4, *except* for strings, which as it turns 
out don't need to be aligned (in SM1 or SM4).

Most of SM1/SM4 deals with bytecode offsets anyway, which means that 
theoretically the native parser could handle unaligned offsets. Since 
almost everything is aligned anyway it's a moot point.

I guess I can dodge the issue by just leaving the alignment behaviour 
alone, though.