Skip to content
Snippets Groups Projects

Various dxbc/spirv refactoring.

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

Merge request reports

Merge request pipeline #3959 skipped

Merge request pipeline skipped for 520c7457

Merged by Alexandre JulliardAlexandre Julliard 2 years ago (Nov 8, 2022 10:05pm 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
  • From patch 1/5:

    I prefer avoiding the vkd3d_* prefix on all internal functions, for these reasons. However, I'm open to restoring it.

    Note that this can make it harder to find the affected code from backtraces and logs, and in some cases bug tracker searches. I.e., suppose you're getting an error message from an application running in wine mentioning "spirv_compiler_some_function()". Does that come from Mesa, vkd3d, wined3d, or perhaps SPIRV-Tools?

    From patch 2/5:

    vkd3d-shader/dxbc: Use the VKD3D_DXBC_MAX_SOURCE_COUNT macro where possible.

    The macro is unfortunately named, but this touches the shader model 4 (TPF) parser, for which we use the "vkd3d-shader/sm4" prefix. Similarly, if we're going to rename this in 3/5, ideally that would be to something like VKD3D_SM4_MAX_SOURCE_COUNT.

    If we're cleaning things up, I'd argue for moving the SM4 parser out of dxbc.c, into its own file, like sm4.c or perhaps tpf.c. I'd also argue for merging hlsl_sm4.c and sm4.h into the same file.

    From patch 4/5:

    -    ins->src_count = strlen(opcode_info->src_info);
    +    ins->src_count = strnlen(opcode_info->src_info, DXBC_MAX_SOURCE_COUNT);

    As it happens, I have a bit of a dislike for the strn*() functions. You'll find very few of them in vkd3d or wined3d...

    From patch 5/5:

    This cuts about 7 kB off of the 64-bit build.

    Is that a debug build, or stripped? Avoiding pointers in struct vkd3d_sm4_opcode_info is generally desirable, but I'd expect the "read_opcode_func" field to be the bigger problem with that. Ideally we'd get rid of the linear search as well...

    -    struct vkd3d_shader_dst_param dst_param[2];
    +    struct vkd3d_shader_dst_param dst_param[DXBC_MAX_DEST_COUNT];

    DXBC_MAX_DEST_COUNT is unfortunate. Like above, I'd prefer VKD3D_SM4_ over DXBC_. However, we also use either "src/dst" or "source/destination"; never "dest".

    Edited by Henri Verbeet
  • From patch 1/5:

    I prefer avoiding the vkd3d_* prefix on all internal functions, for these reasons. However, I'm open to restoring it.

    Note that this can make it harder to find the affected code from backtraces and logs, and in some cases bug tracker searches. I.e., suppose you're getting an error message from an application running in wine mentioning "spirv_compiler_some_function()". Does that come from Mesa, vkd3d, wined3d, or perhaps SPIRV-Tools?

    That strikes me as a reasonable concern, but I think that's something we can better resolve by simply always outputting "vkd3d" in the debug functions. At least, I find that preferable to putting a vkd3d prefix on every internal function.

    From patch 2/5:

    vkd3d-shader/dxbc: Use the VKD3D_DXBC_MAX_SOURCE_COUNT macro where possible.

    The macro is unfortunately named, but this touches the shader model 4 (TPF) parser, for which we use the "vkd3d-shader/sm4" prefix. Similarly, if we're going to rename this in 3/5, ideally that would be to something like VKD3D_SM4_MAX_SOURCE_COUNT.

    Sure.

    If we're cleaning things up, I'd argue for moving the SM4 parser out of dxbc.c, into its own file, like sm4.c or perhaps tpf.c.

    That seems reasonable.

    I'd also argue for merging hlsl_sm4.c and sm4.h into the same file.

    This may also be reasonable, although I think it may depend on more prototyping being done first...

    From patch 4/5:

    -    ins->src_count = strlen(opcode_info->src_info);
    +    ins->src_count = strnlen(opcode_info->src_info, DXBC_MAX_SOURCE_COUNT);

    As it happens, I have a bit of a dislike for the strn*() functions. You'll find very few of them in vkd3d or wined3d...

    I don't understand this one. I suspect the prescription is "use memchr() instead", but that's less convenient. I can see how strncat() and strncpy() are fundamentally broken, but I don't see why the same applies to strnlen(), at least in this case. (Granted, this may be the only usage where strnlen() ever makes sense, but...)

    I suppose the other option is to make the array of size DXBC_MAX_SOURCE_COUNT + 1, but the problem here is that the compiler won't catch it for us if we overflow that array by one.

    From patch 5/5:

    This cuts about 7 kB off of the 64-bit build.

    Is that a debug build, or stripped? Avoiding pointers in struct vkd3d_sm4_opcode_info is generally desirable, but I'd expect the "read_opcode_func" field to be the bigger problem with that. Ideally we'd get rid of the linear search as well...

    It applies to both stripped and unstripped builds. (Actually, the benefit is more like 12 kB on this machine, not sure what changed.)

    I suspect there is a more salient benefit in speed, but I didn't really want to put the effort into measuring that, and the size benefit seemed worthwhile by itself.

    -    struct vkd3d_shader_dst_param dst_param[2];
    +    struct vkd3d_shader_dst_param dst_param[DXBC_MAX_DEST_COUNT];

    DXBC_MAX_DEST_COUNT is unfortunate. Like above, I'd prefer VKD3D_SM4_ over DXBC_. However, we also use either "src/dst" or "source/destination"; never "dest".

    Sure, I'll replace with src/dst.

  • If we're cleaning things up, I'd argue for moving the SM4 parser out of dxbc.c, into its own file, like sm4.c or perhaps tpf.c.

    That seems reasonable.

    I'd also argue for merging hlsl_sm4.c and sm4.h into the same file.

    This may also be reasonable, although I think it may depend on more prototyping being done first...

    Not sure I understand?

    As it happens, I have a bit of a dislike for the strn*() functions. You'll find very few of them in vkd3d or wined3d...

    I don't understand this one. I suspect the prescription is "use memchr() instead", but that's less convenient. I can see how strncat() and strncpy() are fundamentally broken, but I don't see why the same applies to strnlen(), at least in this case. (Granted, this may be the only usage where strnlen() ever makes sense, but...)

    It's probably fine here; it's just not my favourite family of functions.

    This cuts about 7 kB off of the 64-bit build.

    Is that a debug build, or stripped? Avoiding pointers in struct vkd3d_sm4_opcode_info is generally desirable, but I'd expect the "read_opcode_func" field to be the bigger problem with that. Ideally we'd get rid of the linear search as well...

    It applies to both stripped and unstripped builds. (Actually, the benefit is more like 12 kB on this machine, not sure what changed.)

    I suspect there is a more salient benefit in speed, but I didn't really want to put the effort into measuring that, and the size benefit seemed worthwhile by itself.

    In theory it saves some relocations, and the arrays themselves are also smaller than 64-bit pointers. Still, the difference seemed slightly larger than I'd expect. Looking at the number of entries in the table though, perhaps it's not unreasonable.

  • I'd also argue for merging hlsl_sm4.c and sm4.h into the same file.

    This may also be reasonable, although I think it may depend on more prototyping being done first...

    Not sure I understand?

    hlsl_sm4.c in its current state very much mixes sm4 and hlsl definitions. Separating those might be desirable, but will take effort, and the exact lines are in question, especially since there's still open questions about using vkd3d_shader_instruction here.

  • Elizabeth Figura added 6 commits

    added 6 commits

    • 3dcb9d79 - vkd3d-common: Always prefix debug output with "vkd3d".
    • cffdeab9 - vkd3d-shader/spirv: Rename struct vkd3d_dxbc_compiler to struct spirv_compiler.
    • 330eb551 - vkd3d-shader/sm4: Use the VKD3D_DXBC_MAX_SOURCE_COUNT macro where possible.
    • c6b465e2 - vkd3d-shader/spirv: Avoid using DXBC-specific definitions.
    • cbd9f886 - vkd3d-shader/sm4: Use a flat array to store source types.
    • 9f69d1d1 - vkd3d-shader/sm4: Use a flat array to store destination types.

    Compare with previous version

  • hlsl_sm4.c in its current state very much mixes sm4 and hlsl definitions. Separating those might be desirable, but will take effort, and the exact lines are in question, especially since there's still open questions about using vkd3d_shader_instruction here.

    Oh, sure, but that may not need to block moving all the sm4 code to the same file. I.e., in terms of sequence, what I'd imagine would be:

    1. Rename hlsl_sm4.c to sm4.c.
    2. Move the sm4 bits in dxbc.c to sm4.c.
    3. Merge sm4.h into sm4.c.

    It wouldn't necessarily need to involve a lot of changes to the existing code in hlsl_sm4.c.

  • Henri Verbeet approved this merge request

    approved this merge request

  • added 6 commits

    • 58c7c4b8 - vkd3d-common: Always prefix debug output with "vkd3d".
    • 35b48a8b - vkd3d-shader/spirv: Rename struct vkd3d_dxbc_compiler to struct spirv_compiler.
    • ba08825c - vkd3d-shader/sm4: Use the VKD3D_DXBC_MAX_SOURCE_COUNT macro where possible.
    • e2aed385 - vkd3d-shader/spirv: Avoid using DXBC-specific definitions.
    • 4173158c - vkd3d-shader/sm4: Use a flat array to store source types.
    • 520c7457 - vkd3d-shader/sm4: Use a flat array to store destination types.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading