Skip to content
Snippets Groups Projects

vkd3d-shader: Extend vkd3d_shader_scan_descriptor_info, part 1/2.

Merged Elizabeth Figura requested to merge zfigura/vkd3d:mr0 into master

There are two motivations for this:

  • First, the structure almost corresponds to D3D12_SHADER_INPUT_BIND_DESC, and if more elements were included, it could be used as-is for shader reflection.

    There is the quirk that currently we return scan information based on the shader instructions, whereas d3dcompiler shader reflection expects to get it from the shader reflection data (i.e. the RDEF chunk), which is particularly relevant in the case that the RDEF chunk is stripped.

    That said, even if we have to introduce an extra scan API to account for this difference, being able to reuse the same structure seems like a benefit.

    In order to reuse this structure, we need to add the following elements:

    • Register ID (added in part 1 of this series)

    • Sample count (added in part 2 of this series)

    • Flags or resource types to distinguish between typed, raw, and structured buffers. I have not decided which representation makes the most sense; opinions are welcome.

  • Second, I think it makes sense to use this reflection information internally in spirv.c (and potentially other compiler backends) to declare resources in the target environment, instead of parsing DCL instructions. The idea here is that this allows backends to be more agnostic as to how resources are declared (or inferred) in the frontend, while avoiding the need to synthesize those DCL instructions in the frontend either [especially since epenthesizing instructions is more expensive than converting them to NOPs.]

    In order to do that, we will need vkd3d_shader_scan_descriptor_info1 to cover everything that is currently covered by DCL instructions. This needs the same elements as above (register ID and sample count), but also:

    • Structure stride (added in part 2 of this series)

    • Constant buffer used width (added in part 2 of this series)

I don't currently have a proof-of-concept using these new elements. On the other hand, since it's just an extension of an existing API, I figured that seemed less critical.

This does conflict trivially with 280; I'm submitting it now since 280 is accepted, but due to Alexandre's vacation may not be committed soon, and since this is new API I'd rather get comments early anyway.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Broadly, this probably makes sense; there may be an open question whether this should be called vkd3d_shader_scan_descriptor_info1 or something else, and whether we should duplicate the original structure or just provide the extra information separately, but I don't currently have a strong opinion on that.

    I do wonder about the reason for submitting this right now; my impression is that the shader reflection bits aren't in danger of being imminently released, so I imagine this is mostly intended to address the second point. That's a purely internal usage though, and wouldn't require introducing public API.

  • Author Developer

    Broadly, this probably makes sense; there may be an open question whether this should be called vkd3d_shader_scan_descriptor_info1 or something else, and whether we should duplicate the original structure or just provide the extra information separately, but I don't currently have a strong opinion on that.

    I considered not duplicating the original information, but it struck me as awkward, or at least, unidiomatic. It would require a caller to provide both structures instead of just one, and it would also mean they'd have to index two separate arrays.

    As for extending with a -1, well, we do have precedent for that.

    I do wonder about the reason for submitting this right now; my impression is that the shader reflection bits aren't in danger of being imminently released, so I imagine this is mostly intended to address the second point. That's a purely internal usage though, and wouldn't require introducing public API.

    Sure, and I suppose I could just make the structure private for now. I didn't see a reason to do that, though.

  • Broadly, this probably makes sense; there may be an open question whether this should be called vkd3d_shader_scan_descriptor_info1 or something else, and whether we should duplicate the original structure or just provide the extra information separately, but I don't currently have a strong opinion on that.

    I considered not duplicating the original information, but it struck me as awkward, or at least, unidiomatic. It would require a caller to provide both structures instead of just one, and it would also mean they'd have to index two separate arrays.

    As for extending with a -1, well, we do have precedent for that.

    Sure; I guess I'm mostly just saying I haven't given the question much thought, and at some point I probably should.

    I do wonder about the reason for submitting this right now; my impression is that the shader reflection bits aren't in danger of being imminently released, so I imagine this is mostly intended to address the second point. That's a purely internal usage though, and wouldn't require introducing public API.

    Sure, and I suppose I could just make the structure private for now. I didn't see a reason to do that, though.

    Mostly just private structures being easier than public ones, as well as getting the chance to see how using this internally for SPIR-V ends up working out.

  • Author Developer

    I do wonder about the reason for submitting this right now; my impression is that the shader reflection bits aren't in danger of being imminently released, so I imagine this is mostly intended to address the second point. That's a purely internal usage though, and wouldn't require introducing public API.

    Sure, and I suppose I could just make the structure private for now. I didn't see a reason to do that, though.

    Mostly just private structures being easier than public ones, as well as getting the chance to see how using this internally for SPIR-V ends up working out.

    I can see the value in avoiding exposing the API unless necessary. It'd be an awkward and nontrivial bit of extra work to make this internal, though, given how the descriptor info is currently accessed by vkd3d_shader_parser_compile(). Unless I suppose it'd be acceptable to just keep the structures and everything the same but move the struct out of the function header? It'd be a bit awkward, mostly because we'd have a private element of enum vkd3d_shader_structure_type, but less painful.

    Or I could prototype a partial user of this in a reflection API, which broadly shouldn't be too hard anyway. The only really painful part of reflection, and the reason I haven't actually submitted anything yet, is designing the API for constant buffers.

  • I can see the value in avoiding exposing the API unless necessary. It'd be an awkward and nontrivial bit of extra work to make this internal, though, given how the descriptor info is currently accessed by vkd3d_shader_parser_compile().

    Maybe not trivial, but it doesn't seem that hard? In particular, I imagine we can essentially just pass the new structure as optional parameter to scan_with_parser() and use it instead of "local_descriptor_info1" if present.

  • Elizabeth Figura added 10 commits

    added 10 commits

    • 71747b9f...f649db23 - 5 commits from branch wine:master
    • 387a8490 - vkd3d-shader: Factor more code into vkd3d_shader_scan_get_uav_descriptor_info().
    • 61ad650c - vkd3d-shader: Centralize cleanup on error in scan_with_parser().
    • cab33dac - vkd3d-shader: Introduce struct vkd3d_shader_scan_descriptor_info1.
    • 17e89443 - vkd3d-shader: Add register ID to struct vkd3d_shader_descriptor_info1.
    • 90f8b766 - vkd3d-shader: Get rid of the uav_ranges array.

    Compare with previous version

  • Author Developer

    I can see the value in avoiding exposing the API unless necessary. It'd be an awkward and nontrivial bit of extra work to make this internal, though, given how the descriptor info is currently accessed by vkd3d_shader_parser_compile().

    Maybe not trivial, but it doesn't seem that hard? In particular, I imagine we can essentially just pass the new structure as optional parameter to scan_with_parser() and use it instead of "local_descriptor_info1" if present.

    Okay, it's significantly easier than I thought; I don't know what kind of tunnel vision I had that was preventing me from doing it cleanly.

    I've pushed a new version that does this. It also keeps all the current naming, which makes it look like it's still an exported structure. I think this is fine, because I'm still pretty confident we'll want to export this eventually, but I can change it if requested.

  • +static enum vkd3d_result convert_descriptor_info(struct vkd3d_shader_scan_descriptor_info *info,
    +        const struct vkd3d_shader_scan_descriptor_info1 *info1)
    +{
    +    unsigned int i;
    +
    +    if (!(info->descriptors = vkd3d_malloc(info1->descriptor_count * sizeof(*info->descriptors))))
    +        return VKD3D_ERROR_OUT_OF_MEMORY;

    The vkd3d_malloc() call is safe because sizeof(*info->descriptors) <= sizeof(*info1->descriptors), but I don't think it's setting a great example; we'd generally want to use vkd3d_calloc().

  • Elizabeth Figura added 3 commits

    added 3 commits

    • 4ff84627 - vkd3d-shader: Introduce struct vkd3d_shader_scan_descriptor_info1.
    • 075f0fff - vkd3d-shader: Add register ID to struct vkd3d_shader_descriptor_info1.
    • 87e91d5b - vkd3d-shader: Get rid of the uav_ranges array.

    Compare with previous version

  • Author Developer

    The vkd3d_malloc() call is safe because sizeof(*info->descriptors) <= sizeof(*info1->descriptors), but I don't think it's setting a great example; we'd generally want to use vkd3d_calloc().

    Changed in v3. We should probably add vkd3d_malloc_array() and vkd3d_zalloc() à la the kernel.

  • Changed in v3. We should probably add vkd3d_malloc_array() and vkd3d_zalloc() à la the kernel.

    I'm not opposed. :)

  • Henri Verbeet approved this merge request

    approved this merge request

  • Alexandre Julliard added 20 commits

    added 20 commits

    • 87e91d5b...8484bd59 - 15 commits from branch wine:master
    • 9c5cb2de - vkd3d-shader: Factor more code into vkd3d_shader_scan_get_uav_descriptor_info().
    • fd4a820c - vkd3d-shader: Centralize cleanup on error in scan_with_parser().
    • 4e9798f6 - vkd3d-shader: Introduce struct vkd3d_shader_scan_descriptor_info1.
    • 7d029225 - vkd3d-shader: Add register ID to struct vkd3d_shader_descriptor_info1.
    • 983d01df - vkd3d-shader: Get rid of the uav_ranges array.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading