layers:Streamline descriptor mem access update

Put memory access updates for active descriptors directly into the read
and write vectors.
This kills an extra data structure transformation where we moved the
accesses through sets before putting them into vectors. There was no
benefit to the extra overhead and this update brings the performance of
validation with memory access checks on-par with validation when the
checks are disabled.

Still have the warning callback disabled for now.

The ENABLE_MEMORY_ACCESS_CALLBACK define in vk_layer_config.h can be
un-commented to enable the warning callback, as well as the descriptor
memory access tracking.

Author: tobine

layers/core_validation.cpp

@@ -5913,26 +5913,21 @@ static bool ValidateCmdDrawType(layer_data *dev_data, VkCommandBuffer cmd_buffer
         skip |= (VK_PIPELINE_BIND_POINT_GRAPHICS == bind_point) ? outsideRenderPass(dev_data, *cb_state, caller, msg_code)
                                                                 : insideRenderPass(dev_data, *cb_state, caller, msg_code);
 #ifndef ENABLE_MEMORY_ACCESS_CALLBACK
-        // TODO : Early return here to skip the memory access checking below. The checks are functional but cause a perf hit
-        //  and the callback that's used is disabled, so also turning off these checks for now.
+        // Early return here to skip the memory access checking below.
+        //  The callback that's used is disabled, so also turning off these checks for now.
         //  To re-enable the checks, just remove this early return
         return skip;
 #endif
         // Grab mem accesses for this draw & check for missing synchs
         auto const &state = (*cb_state)->lastBound[bind_point];
         PIPELINE_STATE *pPipe = state.pipeline_state;
         if (VK_NULL_HANDLE != state.pipeline_layout.layout) {
-            MemAccessGroup mem_access_group;
             for (const auto &set_binding_pair : pPipe->active_slots) {
                 uint32_t setIndex = set_binding_pair.first;
                 // Pull the set node
                 cvdescriptorset::DescriptorSet *descriptor_set = state.boundDescriptorSets[setIndex];
                 if (descriptor_set) {
-                    // For given active slots record updated images & buffers
-                    descriptor_set->GetReadWriteBuffersAndImages(set_binding_pair.second, &mem_access_group.read_buffers,
-                                                                 &mem_access_group.read_images, &mem_access_group.write_buffers,
-                                                                 &mem_access_group.write_images);
-                    UpdateRWMemoryAccessVectors(dev_data, cmd_type, read_accesses, write_accesses, mem_access_group);
+                    descriptor_set->AddReadWriteBuffersAndImages(set_binding_pair.second, cmd_type, read_accesses, write_accesses);
                 }
             }
             skip |= ValidateRWMemoryAccesses(dev_data->report_data, cmd_buffer, (*cb_state)->mem_accesses, read_accesses,

layers/descriptor_sets.cpp

@@ -555,12 +555,11 @@ bool cvdescriptorset::DescriptorSet::ValidateDrawState(const std::map<uint32_t,
     return true;
 }
 
-// For given bindings, place any update buffers or images into the passed-in unordered_sets
-uint32_t cvdescriptorset::DescriptorSet::GetReadWriteBuffersAndImages(const std::map<uint32_t, descriptor_req> &bindings,
-                                                                      std::unordered_set<VkBuffer> *read_buffer_set,
-                                                                      std::unordered_set<VkImageView> *read_image_set,
-                                                                      std::unordered_set<VkBuffer> *write_buffer_set,
-                                                                      std::unordered_set<VkImageView> *write_image_set) const {
+// For given bindings, place any active buffers or images into the passed-in vectors
+// TODO: Currently just adding entire binding as imprecise update, but ideally would limit access based on view
+uint32_t cvdescriptorset::DescriptorSet::AddReadWriteBuffersAndImages(const std::map<uint32_t, descriptor_req> &bindings,
+                                                                      CMD_TYPE cmd, std::vector<MemoryAccess> *read_access_vector,
+                                                                      std::vector<MemoryAccess> *write_access_vector) const {
     auto num_updates = 0;
     for (auto binding_pair : bindings) {
         auto binding = binding_pair.first;
@@ -569,49 +568,71 @@ uint32_t cvdescriptorset::DescriptorSet::GetReadWriteBuffersAndImages(const std:
             continue;
         }
         auto start_idx = p_layout_->GetGlobalStartIndexFromBinding(binding);
-        auto &buffer_set = read_buffer_set;
-        auto &image_set = read_image_set;
+        const auto descriptor_count = p_layout_->GetDescriptorCountFromBinding(binding);
+        auto &update_vector = read_access_vector;
+        auto update_function = &AddReadMemoryAccess;
         if (descriptors_[start_idx]->IsStorage()) {
-            buffer_set = write_buffer_set;
-            image_set = write_image_set;
+            update_vector = write_access_vector;
+            update_function = &AddWriteMemoryAccess;
         }
         switch (descriptors_[start_idx]->descriptor_class) {
             case (Image): {
-                for (uint32_t i = 0; i < p_layout_->GetDescriptorCountFromBinding(binding); ++i) {
+                for (uint32_t i = 0; i < descriptor_count; ++i) {
                     if (descriptors_[start_idx + i]->updated) {
-                        image_set->insert(static_cast<ImageDescriptor *>(descriptors_[start_idx + i].get())->GetImageView());
-                        num_updates++;
+                        auto image_view = static_cast<ImageDescriptor *>(descriptors_[start_idx + i].get())->GetImageView();
+                        auto image_view_state = GetImageViewState(device_data_, image_view);
+                        if (image_view_state) {
+                            auto image_node = GetImageState(device_data_, image_view_state->create_info.image);
+                            if (image_node) {
+                                update_function(cmd, update_vector, image_node->binding, false);
+                                num_updates++;
+                            }
+                        }
                     }
                 }
                 break;
             }
             case (ImageSampler): {
-                for (uint32_t i = 0; i < p_layout_->GetDescriptorCountFromBinding(binding); ++i) {
+                for (uint32_t i = 0; i < descriptor_count; ++i) {
                     if (descriptors_[start_idx + i]->updated) {
-                        image_set->insert(static_cast<ImageSamplerDescriptor *>(descriptors_[start_idx + i].get())->GetImageView());
-                        num_updates++;
+                        auto image_view = static_cast<ImageSamplerDescriptor *>(descriptors_[start_idx + i].get())->GetImageView();
+                        auto image_view_state = GetImageViewState(device_data_, image_view);
+                        if (image_view_state) {
+                            auto image_node = GetImageState(device_data_, image_view_state->create_info.image);
+                            if (image_node) {
+                                update_function(cmd, update_vector, image_node->binding, false);
+                                num_updates++;
+                            }
+                        }
                     }
                 }
                 break;
             }
             case (TexelBuffer): {
-                for (uint32_t i = 0; i < p_layout_->GetDescriptorCountFromBinding(binding); ++i) {
+                for (uint32_t i = 0; i < descriptor_count; ++i) {
                     if (descriptors_[start_idx + i]->updated) {
                         auto bufferview = static_cast<TexelDescriptor *>(descriptors_[start_idx + i].get())->GetBufferView();
                         auto bv_state = GetBufferViewState(device_data_, bufferview);
                         if (bv_state) {
-                            buffer_set->insert(bv_state->create_info.buffer);
-                            num_updates++;
+                            auto buff_state = GetBufferState(device_data_, bv_state->create_info.buffer);
+                            if (buff_state) {
+                                update_function(cmd, update_vector, buff_state->binding, false);
+                                num_updates++;
+                            }
                         }
                     }
                 }
                 break;
             }
             case (GeneralBuffer): {
-                for (uint32_t i = 0; i < p_layout_->GetDescriptorCountFromBinding(binding); ++i) {
+                for (uint32_t i = 0; i < descriptor_count; ++i) {
                     if (descriptors_[start_idx + i]->updated) {
-                        buffer_set->insert(static_cast<BufferDescriptor *>(descriptors_[start_idx + i].get())->GetBuffer());
-                        num_updates++;
+                        auto buff_state = GetBufferState(
+                            device_data_, static_cast<BufferDescriptor *>(descriptors_[start_idx + i].get())->GetBuffer());
+                        if (buff_state) {
+                            update_function(cmd, update_vector, buff_state->binding, false);
+                            num_updates++;
+                        }
                     }
                 }
                 break;

layers/descriptor_sets.h

@@ -347,12 +347,10 @@ class DescriptorSet : public BASE_NODE {
     // For given bindings validate state at time of draw is correct, returning false on error and writing error details into string*
     bool ValidateDrawState(const std::map<uint32_t, descriptor_req> &, const std::vector<uint32_t> &, const GLOBAL_CB_NODE *,
                            const char *caller, std::string *) const;
-    // For given set of bindings, add any buffers and images that will be updated to their respective unordered_sets & return number
-    // of objects inserted
-    uint32_t GetReadWriteBuffersAndImages(const std::map<uint32_t, descriptor_req> &, std::unordered_set<VkBuffer> *read_buffer_set,
-                                          std::unordered_set<VkImageView> *read_image_set,
-                                          std::unordered_set<VkBuffer> *write_buffer_set,
-                                          std::unordered_set<VkImageView> *write_image_set) const;
+    // For given set of bindings, add memory bindings for buffers and images that will be updated to respective read & write vectors
+    uint32_t AddReadWriteBuffersAndImages(const std::map<uint32_t, descriptor_req> &bindings, CMD_TYPE cmd,
+                                          std::vector<MemoryAccess> *read_access_vector,
+                                          std::vector<MemoryAccess> *write_access_vector) const;
 
     // Descriptor Update functions. These functions validate state and perform update separately
     // Validate contents of a WriteUpdate

layers/vk_layer_config.h

@@ -29,6 +29,8 @@
 extern "C" {
 #endif
 
+//#define ENABLE_MEMORY_ACCESS_CALLBACK
+
 // Definitions for Debug Actions
 typedef enum VkLayerDbgActionBits {
     VK_DBG_LAYER_ACTION_IGNORE = 0x00000000,