KhronosGroup / SPIRV-Cross

SPIRV-Cross is a practical tool and library for performing reflection on SPIR-V and disassembling SPIR-V back to high level languages.
Apache License 2.0
2.01k stars 556 forks source link

spirv-cross --reflect resource group namings are confusing and imprecise #723

Open spencerkohan opened 5 years ago

spencerkohan commented 5 years ago

The way spirv-cross --reflect exposes type information about bound resources is currently very confusing. In the JSON output we get a list of arrays whose names are the only way to get which type of descriptor is bound, but the names are only somewhat related to the descriptor types. For instance:

{
    "textures" : [ ... ],
    "separate_images" : [ ... ],
    "images" :  [ ... ]
}

The only way I can be sure that images refers to VK_DESCRIPTOR_TYPE_STORAGE_IMAGE descriptors rather than VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE is to view the source of SPIRV-cross:

void CompilerReflection::emit_resources()
{
auto res = get_shader_resources();
emit_resources("subpass_inputs", res.subpass_inputs);
emit_resources("inputs", res.stage_inputs);
emit_resources("outputs", res.stage_outputs);
emit_resources("textures", res.sampled_images);
emit_resources("separate_images", res.separate_images);
emit_resources("separate_samplers", res.separate_samplers);
emit_resources("images", res.storage_images);
emit_resources("ssbos", res.storage_buffers);
emit_resources("ubos", res.uniform_buffers);
emit_resources("push_constants", res.push_constant_buffers);
emit_resources("counters", res.atomic_counters);
}

There is a well-defined and finite set of descriptor types, why not name the arrays explicitly after those types? Or better yet have a single array of bound_resources each with a descriptor_type field which contains the Vulkan descriptor type constant?

HansKristian-Work commented 5 years ago

Emitting the JSON resources in terms of their Vulkan descriptor types seems like a good idea. I agree it's not obvious how the mapping works. @jherico, would you object to this change?

jherico commented 5 years ago

The vulkan descriptor types are just integers, which would make the format more opaque, not less. If you use string versions of the C constants, it's more obvious, but you'll still need to create a string to integer mapping somewhere.

I don't have any objection to makeing the json output more precise or or more self documenting, but if the format is going to change we should consider either actively soliciting consensus on what the format should be or consider versioning the output, since developers won't use the json if it's a moving target.

HansKristian-Work commented 5 years ago

I guess the goal would be something more self-documenting by using the string types by name. The difference between image load/store, sampled images, etc, are non-obvious.

The mapping is documented for the API at least: https://github.com/KhronosGroup/SPIRV-Cross/wiki/Reflection-API-user-guide#mapping-reflection-output-to-vulkan-cheat-sheet. The JSON output looks like a 1:1 match, so this might be good enough documentation?

However, mapping to descriptor type isn't a pure name replacement, texel buffers need more poking to figure out due to the way the SPIR-V type system works.

The JSON backend is still marked as EXPERIMENTAL, so I don't really fear breaking changes if we can make it easier to use, but if this is already is use by more than a few developers, I don't want to break it just for the sake of it either.

The first questions to answer is:

jherico commented 5 years ago

One alternative would be to add an additional command line flag that replaces the strings with the integer identifiers of the Vulkan enums if the flag is present.