Consider member-level nonwritable decorations #384
Conversation
|
@zeux - pull request is intended to address your issue. If you had the time to test vs. your use case this would be greatly appreciated. |
| } | ||
|
|
||
| case spv::OpTypeStruct: | ||
| case spv::OpTypeStruct: { |
There was a problem hiding this comment.
Do we need to recur into struct member types for full coverage, or is this code guarantee to be at a leaf type? For example if a contained struct wasn't decorated as non-writable, but all of it's members were...
There was a problem hiding this comment.
According to my reading of the spec, this only applies to direct members of block types; The struct we're encountering here is the block itself.
GLSL disallows memory qualifiers on members of normal structs, even if that struct is then used in a block declaration.
layers/shader_validation.cpp
Outdated
| } | ||
| } | ||
|
|
||
| // A buffer is writable if it's either flavor of storage buffer, and has any member not decorateddoesn't have all of its |
There was a problem hiding this comment.
Looks like an incomplete comment update here. Also did you clang-format as this looks a bit long.
There was a problem hiding this comment.
Passes clang-format. Something weird happened here, though...
layers/shader_validation.cpp
Outdated
| @@ -766,7 +776,8 @@ static std::vector<std::pair<descriptor_slot_t, interface_var>> CollectInterface | |||
| v.type_id = insn.word(1); | |||
| out.emplace_back(std::make_pair(set, binding), v); | |||
|
|
|||
| if (var_nonwritable.find(id) == var_nonwritable.end() && IsWritableDescriptorType(src, insn.word(1))) { | |||
| if (IsWritableDescriptorType(src, insn.word(1), insn.word(3) == spv::StorageClassStorageBuffer, | |||
| var_nonwritable.find(id) != var_nonwritable.end())) { | |||
There was a problem hiding this comment.
If we know at this point that the entire object is non-writable can't we skip the call down to IsWriteableDescriptorType? I suspect there's a reason that you refactored to pass this info to the function but it's not clear to me.
There was a problem hiding this comment.
I'm going to undo this. It made a lot more sense in some of the early versions of the change.
Previously we only cared about whether the NonWritable decoration was applied to an entire descriptor-backed OpVariable. This is problematic for real-world shaders, because glslang emits per-member decorations instead in all cases. Adjust the logic to consider per-member decorations in these cases. Fixes #73.
chrisforbes
force-pushed
the
nonwritable-member-level
branch
from
0adbbcd to
cb154ff
Compare
|
Updated. |
|
I had some ideas while writing this about the direction to go for per-descriptor NonWritable and NonReadable tracking which we'll need "sometime", but adjusted this patch to just do exactly what it needs to for now. |
Previously we only cared about whether the NonWritable decoration was
applied to an entire descriptor-backed OpVariable. This is problematic
for real-world shaders, because glslang emits per-member decorations
instead in all cases.
Adjust the logic to consider per-member decorations in these cases.
Fixes #73.