vkd3d-shader: Extend vkd3d_shader_scan_descriptor_info, part 2/2.
This adds all of the information used by the spirv backend to declare descriptor variables. Most of these are added to the internal vkd3d_shader_scan_descriptor_info1 structure, but patch 5/5 adds a flag to enum vkd3d_shader_descriptor_info_flag, which is public API.
The ultimate goal here is to declare SPIR-V descriptor variables from the scanned descriptor information, thereby avoiding the need to synthesize DCL instructions for frontends that don't have them (specifically sm1, but it may be that sm6 would benefit from this as well).
That work is visible here:
https://gitlab.winehq.org/zfigura/vkd3d/-/tree/himavant5
--
This is a follow-up to the already-approved 295, which I am submitting now so that we're not blocked on Alexandre's vacation.
Merge request reports
Activity
From 8f68f5bc759411667c1db9a050aa9ba2ebb5a958 Mon Sep 17 00:00:00 2001 From: Zebediah Figura <zfigura@codeweavers.com> Date: Mon, 31 Jul 2023 19:52:16 -0500 Subject: [PATCH 06/10] vkd3d-shader: Pass a complete vkd3d_shader_descriptor_info1 to vkd3d_shader_scan_add_descriptor(). Adding more parameters, especially bare numeric parameters, gets unwieldy.
Sure, though for the things added in this MR, we could also do something like this:
descriptor = vkd3d_shader_scan_add_descriptor(...); descriptor->sample_count = sample_count; descriptor->structure_stride = structure_stride;
or take it a step further, and do this:
descriptor = vkd3d_shader_scan_resource_declaration(...); descriptor->sample_count = semantic->sample_count; ... descriptor = vkd3d_shader_scan_resource_declaration(...); descriptor->structure_stride = instruction->declaration.structured_resource.byte_stride;
and so on. I don't have a strong preference, but the resulting code may be slightly more compact, and the second variant would avoid passing some optional parameters through vkd3d_shader_scan_resource_declaration() as well.
+ /** The descriptor is a raw (byte-addressed) buffer. */ + VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_RAW_BUFFER = 0x00000010,
That needs a \since.
From 8f68f5bc759411667c1db9a050aa9ba2ebb5a958 Mon Sep 17 00:00:00 2001 From: Zebediah Figura <zfigura@codeweavers.com> Date: Mon, 31 Jul 2023 19:52:16 -0500 Subject: [PATCH 06/10] vkd3d-shader: Pass a complete vkd3d_shader_descriptor_info1 to vkd3d_shader_scan_add_descriptor(). Adding more parameters, especially bare numeric parameters, gets unwieldy.
Sure, though for the things added in this MR, we could also do something like this:
descriptor = vkd3d_shader_scan_add_descriptor(...); descriptor->sample_count = sample_count; descriptor->structure_stride = structure_stride;
or take it a step further, and do this:
descriptor = vkd3d_shader_scan_resource_declaration(...); descriptor->sample_count = semantic->sample_count; ... descriptor = vkd3d_shader_scan_resource_declaration(...); descriptor->structure_stride = instruction->declaration.structured_resource.byte_stride;
and so on. I don't have a strong preference, but the resulting code may be slightly more compact, and the second variant would avoid passing some optional parameters through vkd3d_shader_scan_resource_declaration() as well.
Personally I have a hard time seeing this as idiomatic, or preferable to the approach taken in this commit, but I can see the advantage, so if it's a weak preference I'll go ahead and change it accordingly.
+ /** The descriptor is a raw (byte-addressed) buffer. */ + VKD3D_SHADER_DESCRIPTOR_INFO_FLAG_RAW_BUFFER = 0x00000010,
That needs a \since.
Oops, thanks for the catch.
Personally I have a hard time seeing this as idiomatic, or preferable to the approach taken in this commit,
Mostly out of curiosity, is there anything in particular that strikes you as unidiomatic?
We are making certain trade-offs, of course; the existing code allocates the new descriptor info, fully initialises it, and leaves it immutable afterwards. There are advantages to that, and I think it works well enough for the relatively few parameters we have in the existing code. It leaves little room for optional/default parameters though; callers like vkd3d_shader_scan_constant_buffer_declaration() and vkd3d_shader_scan_resource_declaration() that don't need flags still need to explicitly specify them as "0". 06/10 essentially preserves that, but uses struct vkd3d_shader_descriptor_info1 to pass the parameters. The other extreme would be to have vkd3d_shader_scan_add_descriptor() only allocate the storage, and making the callers responsible for initialising everything. You could imagine a caller like vkd3d_shader_scan_constant_buffer_declaration() then doing something like this:
info = vkd3d_shader_scan_add_descriptor(context); vkd3d_shader_descriptor_info_init_cbv(info, cb, cb->size);
with vkd3d_shader_descriptor_info_init_cbv() doing something like this:
vkd3d_shader_descriptor_info_init(info, VKD3D_SHADER_DESCRIPTOR_TYPE_CBV, &cb->src.reg, &cb->range, VKD3D_SHADER_RESOURCE_BUFFER, VKD3D_SHADER_RESOURCE_DATA_UINT); info->buffer_size = vec4_count * 16;
The proposed alternative doesn't go quite that far, but it shifts things a little bit more in that direction.
but I can see the advantage, so if it's a weak preference I'll go ahead and change it accordingly.
I think it would be slightly nicer, yes.
Something that's perhaps also worth pointing out: Now that we have ir.c, most of the vkd3d_shader_scan() code would perhaps be more appropriate there than in vkd3d_shader_main.c. I'm not sure if moving it is worth it, but if we are going to move it, it's probably easier to do that before adding much to it.
Mostly out of curiosity, is there anything in particular that strikes you as unidiomatic?
As you probably were anticipating, that it initializes the descriptor in two halves. I don't hate it of course, I just don't find it maximally idiomatic.
Something that's perhaps also worth pointing out: Now that we have ir.c, most of the vkd3d_shader_scan() code would perhaps be more appropriate there than in vkd3d_shader_main.c. I'm not sure if moving it is worth it, but if we are going to move it, it's probably easier to do that before adding much to it.
I was actually thinking that at least some of the code was probably going to belong in the individual frontends. I don't think any of the frontends declare descriptors in the same way, at least, so it hardly makes sense to me to have that in common code. The control flow scanning probably makes sense in ir.c, though.
I was actually thinking that at least some of the code was probably going to belong in the individual frontends. I don't think any of the frontends declare descriptors in the same way, at least, so it hardly makes sense to me to have that in common code. The control flow scanning probably makes sense in ir.c, though.
Sure, it would probably make sense to build e.g. descriptor information in the frontends while parsing. (Although, would TPF and DXIL really end up being that different from each other?) As it is though, the vkd3d_shader_scan_instruction() loop in particular just operates on the IR.
added 33 commits
-
ab8df346...4f2e07a4 - 28 commits from branch
wine:master
- 3d429fb2 - vkd3d-shader: Set descriptor flags in the caller to vkd3d_shader_scan_add_descriptor().
- 8640fb21 - vkd3d-shader: Add sample count to struct vkd3d_shader_descriptor_info1.
- 33bc09e1 - vkd3d-shader: Add constant buffer size to struct vkd3d_shader_descriptor_info1.
- 4f7d63f4 - vkd3d-shader: Add structure stride to struct vkd3d_shader_descriptor_info1.
- 12e4c308 - vkd3d-shader: Add a flag marking raw buffers to struct vkd3d_shader_descriptor_info.
Toggle commit list-
ab8df346...4f2e07a4 - 28 commits from branch
@@ -777,11 +777,10 @@ static bool vkd3d_shader_scan_add_descriptor(struct vkd3d_shader_scan_context *c d->register_index = range->first; d->resource_type = resource_type; d->resource_data_type = resource_data_type; - d->flags = flags; d->count = (range->last == ~0u) ? ~0u : range->last - range->first + 1; ++info->descriptor_count; - return true; + return d; } static void vkd3d_shader_scan_constant_buffer_declaration(struct vkd3d_shader_scan_context *context,
You can't quite do that like this though. As it is, this leaves d->flags uninitialised. We either need to initialise it in vkd3d_shader_scan_add_descriptor(), or in each of the callers. That's an issue in the later patches in this MR as well, and for me at least the tests catch it.
mentioned in merge request !306 (closed)
added 5 commits
- 00356cd3 - vkd3d-shader: Set descriptor flags in the caller to vkd3d_shader_scan_add_descriptor().
- 10b61194 - vkd3d-shader: Add sample count to struct vkd3d_shader_descriptor_info1.
- 9bade568 - vkd3d-shader: Add constant buffer size to struct vkd3d_shader_descriptor_info1.
- 6c3771d0 - vkd3d-shader: Add structure stride to struct vkd3d_shader_descriptor_info1.
- 6311e812 - vkd3d-shader: Add a flag marking raw buffers to struct vkd3d_shader_descriptor_info.
Toggle commit listYou can't quite do that like this though. As it is, this leaves d->flags uninitialised. We either need to initialise it in vkd3d_shader_scan_add_descriptor(), or in each of the callers. That's an issue in the later patches in this MR as well, and for me at least the tests catch it.
Ugh, yes, thanks for catching that. That's what I get for submitting revisions without running the tests each time.
added 8 commits
-
6311e812...f3baf55d - 3 commits from branch
wine:master
- ccedb7f7 - vkd3d-shader: Set descriptor flags in the caller to vkd3d_shader_scan_add_descriptor().
- 8c465c81 - vkd3d-shader: Add sample count to struct vkd3d_shader_descriptor_info1.
- 88f85ffb - vkd3d-shader: Add constant buffer size to struct vkd3d_shader_descriptor_info1.
- c1ebba95 - vkd3d-shader: Add structure stride to struct vkd3d_shader_descriptor_info1.
- 622311da - vkd3d-shader: Add a flag marking raw buffers to struct vkd3d_shader_descriptor_info.
Toggle commit list-
6311e812...f3baf55d - 3 commits from branch