remove has_capability() checks#531
Conversation
Firestar99
force-pushed
the
remove_has_capability
branch
3 times, most recently
from
c38ccd4 to
784c8c8
Compare
Firestar99
force-pushed
the
remove_has_capability
branch
2 times, most recently
from
ddbc0c2 to
3874f69
Compare
| if self.builder.has_capability(Capability::ShaderNonUniform) { | ||
| // apply NonUniform to the operation and the index | ||
| self.emit() | ||
| .decorate(ptr.def(self), Decoration::NonUniform, []); | ||
| self.emit() | ||
| .decorate(actual_index, Decoration::NonUniform, []); | ||
| } | ||
| // apply NonUniform to the operation and the index | ||
| self.emit() | ||
| .decorate(ptr.def(self), Decoration::NonUniform, []); | ||
| self.emit() | ||
| .decorate(actual_index, Decoration::NonUniform, []); |
There was a problem hiding this comment.
Can you add a test using this feature, but not enabling ShaderNonUniform?
It should trigger a validation error from the new SPIR-T-based validation pass, if I'm not mistaken.
There was a problem hiding this comment.
Done, works without any issues. I seem to have added tests for everything except ByteAddressableBuffer.
There was a problem hiding this comment.
To be clear, I am talking about a test that lacks the ShaderNonUniform capability, and therefore results in an error. If it compiles successfully and contains NonUniform decorations, without the necessary capability, that's a bug IMO.
There was a problem hiding this comment.
If you look at the tests this PR adds, there's a ByteAddressableBuffer variant with NonUniform capability, and a ByteAddressableBufferUniform variant without, all disassembled so you can diff that and see the the uniform variant missing the OpDecoration %x NonUniform
There was a problem hiding this comment.
But you removed the condition that gates NonUniform on ShaderNonUniform, and the decoration should always be emitted now... is something else removing the NonUniform decorations?
There was a problem hiding this comment.
Yea, here a linker pass that removes all NonUniform decorations, which has been added in #177 1.5 years ago. I wanted it so I could write just one image access asm! block with all the needed NonUniform decorations, since you can't conditionally exclude an asm line.
Firestar99
force-pushed
the
remove_has_capability
branch
from
3874f69 to
98b816b
Compare
Firestar99
force-pushed
the
remove_has_capability
branch
from
98b816b to
2ee060d
Compare
2 participants
BuilderSpirv::has_capability(&self, capability: Capability) -> bool, you can no longer check whether some capability is enabled. Progress towards RFC: link-time capabilities #388has_capabilitycheck)