gpu: Remove Alias support to suppress false positives #8879
Conversation
|
CI Vulkan-ValidationLayers build queued with queue ID 305289. |
| @@ -152,6 +149,12 @@ bool DescriptorValidator::ValidateBindingDynamic(const DescriptorBindingInfo &bi | |||
| auto binding_ptr = descriptor_set.GetBinding(binding_info.first); | |||
| ASSERT_AND_RETURN_SKIP(binding_ptr); | |||
| auto &binding = *binding_ptr; | |||
|
|
|||
| // If false, we have already validated (and updated the state) for this descriptor on the CPU | |||
| if (!descriptor_set.ValidateBindingOnGPU(binding, binding_info.second.variable->is_dynamic_accessed)) { | |||
There was a problem hiding this comment.
this is actually a BIG motivation for this change. I want to make the exact same call we do in the CPU to defer work off to the GPU to also no repeat work
Some false positives are things that we were validating on the CPU, just to have it re-run ValidateDescriptor in GPU-AV and get it wrong
| @@ -238,61 +239,57 @@ bool DescriptorValidator::ValidateDescriptor(const DescriptorBindingInfo &bindin | |||
| return false; | |||
| } | |||
|
|
|||
| static const spirv::ResourceInterfaceVariable *FindMatchingImageVariable(const std::vector<DescriptorRequirement> &reqs, | |||
| static const spirv::ResourceInterfaceVariable *FindMatchingImageVariable(const DescriptorRequirement &descriptor_req, | |||
There was a problem hiding this comment.
this function (and FindMatchingTexelVariable) is strange, I "think" it could be removed, but wanted to try that in another PR as it might uncover some other issue
There was a problem hiding this comment.
the tests failing are showing me that there is actually just some larger problems at play... glad we had tests, but things are kinda a mess right now trying to share this ValidateDescriptor on the CPU/GPU and having subtle things in it only for one or the other code path
| context.ValidateBindingDynamic(binding_info, u.second); | ||
| // TODO - https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/8875 | ||
| // Currently if we find multiple variables (aka aliased bindings) we will skip validating | ||
| if (descriptor_req_found == 1) { |
There was a problem hiding this comment.
I am not sure if this can be zero, it shouldn't be able to, but will need more testing
| &descriptor_set.set_, 0, nullptr); | ||
| vk::CmdBindPipeline(m_command_buffer.handle(), VK_PIPELINE_BIND_POINT_GRAPHICS, g_pipe.Handle()); | ||
|
|
||
| if (!m_gpuav_disable_core) { |
There was a problem hiding this comment.
we had these in some test... this was sign that this is actually just validated on the CPU and doesn't need GPU-AV
|
CI Vulkan-ValidationLayers build # 18053 running. |
|
CI Vulkan-ValidationLayers build # 18053 failed. |
| @@ -16,6 +16,7 @@ | |||
| */ | |||
|
|
|||
| #include "gpu/descriptor_validation/gpuav_descriptor_validation.h" | |||
| #include <cstdint> | |||
| while (iter != bound_descriptor_set.binding_req_map.end() && iter->first == binding) { | ||
| binding_info.second.emplace_back(iter->second); | ||
| descriptor_req_found++; | ||
| binding_info.second = iter->second; |
There was a problem hiding this comment.
Needless vector copies will be done here, just have one copy during the last iteration
Also, do we truly need a deep copy?
| @@ -983,7 +983,7 @@ class DescriptorSet : public StateObject { | |||
| return DescriptorIterator<ConstBindingIterator>(*this, binding, index); | |||
| } | |||
|
|
|||
| virtual bool SkipBinding(const DescriptorBinding &binding, bool is_dynamic_accessed) const { | |||
| inline bool ValidateBindingOnGPU(const DescriptorBinding &binding, bool is_dynamic_accessed) const { | |||
There was a problem hiding this comment.
| inline bool ValidateBindingOnGPU(const DescriptorBinding &binding, bool is_dynamic_accessed) const { | |
| bool ValidateBindingOnGPU(const DescriptorBinding &binding, bool is_dynamic_accessed) const { |
Function defined in a header are likely to be inlined anyway, and inline being just a hint, just let the compiler decide!
There was a problem hiding this comment.
Yeah, inline is mostly outdated now, compilers inline not only little but medium sized functions too, results in mostly large functions in the output, and function calls are less expensive as a result 🙃
There was a problem hiding this comment.
I guess another part of inline that deals with One Definition rule can more central reason to use it
We added a quick hack for #8875 in #8876, but this takes it a step further and just removes alias binding checks all together. This will allow us to more easily get other things correct, and then can return and try to apply them back in