layers: Rework creating a spirv::Module object

Author: spencer-lunarg

layers/VkLayer_khronos_validation.json.in

@@ -313,7 +313,7 @@
                                 {
                                     "key": "check_shaders",
                                     "label": "Shader",
-                                    "description": "This will validate the contents of the SPIR-V which can be CPU intensive during application start up. This does internal checks as well as calling spirv-val.",
+                                    "description": "This will validate the contents of the SPIR-V which can be CPU intensive during application start up. This does internal checks as well as calling spirv-val. (Same effect using VK_VALIDATION_FEATURE_DISABLE_SHADERS_EXT)",
                                     "type": "BOOL",
                                     "default": true,
                                     "expanded": true,

layers/core_checks/cc_shader_object.cpp

@@ -287,9 +287,9 @@ bool CoreChecks::PreCallValidateCreateShadersEXT(VkDevice device, uint32_t creat
                                                  VkShaderEXT* pShaders, const ErrorObject& error_obj) const {
     bool skip = false;
 
-    // the spec clarifies that VK_VALIDATION_FEATURE_DISABLE_SHADERS_EXT works on VK_EXT_shader_object as well
+    // If user has VK_VALIDATION_FEATURE_DISABLE_SHADERS_EXT, just skip all things related to creating the shader object
     if (disabled[shader_validation]) {
-        return skip; // VK_VALIDATION_FEATURE_DISABLE_SHADERS_EXT
+        return skip;
     }
 
     if (enabled_features.shaderObject == VK_FALSE) {
@@ -323,7 +323,8 @@ bool CoreChecks::PreCallValidateCreateShadersEXT(VkDevice device, uint32_t creat
         spv_const_binary_t binary{static_cast<const uint32_t*>(create_info.pCode), create_info.codeSize / sizeof(uint32_t)};
         skip |= RunSpirvValidation(binary, create_info_loc, cache);
 
-        const auto spirv = std::make_shared<spirv::Module>(create_info.codeSize, static_cast<const uint32_t*>(create_info.pCode));
+        const auto spirv =
+            vvl::CreateSpirvModuleState(create_info.codeSize, static_cast<const uint32_t*>(create_info.pCode), global_settings);
         vku::safe_VkShaderCreateInfoEXT safe_create_info = vku::safe_VkShaderCreateInfoEXT(&pCreateInfos[i]);
         const ShaderStageState stage_state(nullptr, &safe_create_info, nullptr, spirv);
         skip |= ValidateShaderStage(stage_state, nullptr, create_info_loc);

layers/core_checks/cc_spirv.cpp

@@ -1774,12 +1774,21 @@ bool CoreChecks::ValidateShaderStage(const ShaderStageState &stage_state, const
         }
     }
 
+    // Skip if VK_VALIDATION_FEATURE_DISABLE_SHADERS_EXT is set
+    // Both the validation and running spirv-opt on the spec constants really makes this function slow
+    // See https://github.com/KhronosGroup/Vulkan-ValidationLayers/pull/10566 for more info
+    if (disabled[shader_validation]) {
+        return skip;
+    }
+
     if ((pipeline && pipeline->uses_shader_module_id) || !stage_state.spirv_state) {
         return skip;  // these edge cases should be validated already
     }
 
     const spirv::Module &module_state = *stage_state.spirv_state.get();
-    if (!module_state.valid_spirv) return skip;  // checked elsewhere
+    if (!module_state.valid_spirv) {
+        return skip;  // checked elsewhere
+    }
 
     if (!stage_state.entrypoint) {
         const char *vuid = pipeline ? "VUID-VkPipelineShaderStageCreateInfo-pName-00707" : "VUID-VkShaderCreateInfoEXT-pName-08440";
@@ -1814,8 +1823,7 @@ bool CoreChecks::ValidateShaderStage(const ShaderStageState &stage_state, const
     uint32_t total_task_payload_memory = 0;
 
     // If specialization-constant instructions are present in the shader, the specializations should be applied.
-    // Skip if VK_VALIDATION_FEATURE_DISABLE_SHADERS_EXT is set.
-    if (!disabled[shader_validation] && module_state.static_data_.has_specialization_constants) {
+    if (module_state.static_data_.has_specialization_constants && global_settings.spirv_const_fold) {
         // setup the call back if the optimizer fails
         spv_target_env spirv_environment = PickSpirvEnv(api_version, IsExtEnabled(extensions.vk_khr_spirv_1_4));
         spvtools::Optimizer optimizer(spirv_environment);
@@ -2133,10 +2141,8 @@ void CoreChecks::PreCallRecordCreateShaderModule(VkDevice device, const VkShader
                                                  const RecordObject &record_obj, chassis::CreateShaderModule &chassis_state) {
     // Normally would validate in PreCallValidate, but need a non-const function to update chassis_state
     // This is on the stack, we don't have to worry about threading hazards and this could be moved and used const_cast
-    if (chassis_state.module_state) {
-        chassis_state.skip |=
-            stateless_spirv_validator.Validate(*chassis_state.module_state, chassis_state.stateless_data, record_obj.location);
-    }
+    chassis_state.skip |=
+        stateless_spirv_validator.Validate(*chassis_state.module_state, chassis_state.stateless_data, record_obj.location);
 }
 
 void CoreChecks::PreCallRecordCreateShadersEXT(VkDevice device, uint32_t createInfoCount, const VkShaderCreateInfoEXT *pCreateInfos,

layers/core_checks/core_validation.h

@@ -222,7 +222,7 @@ class CoreChecks : public vvl::DeviceProxy {
 
     CoreChecks(vvl::dispatch::Device* dev, core::Instance* instance_vo)
         : BaseClass(dev, instance_vo, LayerObjectTypeCoreValidation),
-          stateless_spirv_validator(dev->debug_report, dev->stateless_device_data) {}
+          stateless_spirv_validator(dev->debug_report, dev->stateless_device_data, dev->settings.disabled[shader_validation]) {}
 
     ReadLockGuard ReadLock() const override;
     WriteLockGuard WriteLock() override;

layers/layer_options.cpp

@@ -1119,6 +1119,28 @@ void ProcessConfigAndEnvSettings(ConfigAndEnvSettings *settings_data) {
             "errors in Core Check are solved, please disable, then only use GPU-AV for best performance.");
     }
 
+    // Set at the end once we decide what settings are actually on
+    if (settings_data->disables[shader_validation] || settings_data->disables[core_checks]) {
+        // only is used for core validation checks
+        global_settings.spirv_const_fold = false;
+
+        // sync val relies on the information from spirv::Module being parsed and stored
+        if (!settings_data->enables[sync_validation]) {
+            // GPU-AV/DebugPrintf relies on spirv::Module to hold the original SPIR-V
+            if (!settings_data->enables[gpu_validation] && !settings_data->enables[debug_printf_validation]) {
+                global_settings.spirv_store = false;
+            } else if (settings_data->disables[shader_validation]) {
+                setting_warnings.emplace_back(
+                    "Shader Validation was explicitly turned off, but the SPIR-V still needs to be stored, but not validated, in "
+                    "order for GPU-AV/DebugPrintf to get information from the original SPIR-V.");
+            }
+        } else if (settings_data->disables[shader_validation]) {
+            setting_warnings.emplace_back(
+                "Shader Validation was explicitly turned off, but the SPIR-V still needs to be parsed/stored, but not validated, "
+                "in order for Sync Validation to get read/write information out the SPIR-V.");
+        }
+    }
+
     // Last as previous settings are needed so we can make sure they line up with the DebugReport settings
     ProcessDebugReportSettings(settings_data, layer_setting_set, setting_warnings);
 

layers/layer_options.h

@@ -84,6 +84,20 @@ struct GlobalSettings {
     bool fine_grained_locking = true;
 
     bool debug_disable_spirv_val = false;
+
+    // We have the spirv::Module state object designed to hold the SPIR-V and parse it so we can do validation on it, the
+    // issue/tricky part is gpuav/debugPrint use this to get the original SPIR-V (which we need to provide any useful error
+    // messages). These settings are here to make it more obvious what we want at runtime
+    //
+    // Stores a copy, needed to provide error messages and how DebugPrintf works
+    // (performance is fast, but memory overhead is higher)
+    bool spirv_store = true;
+    // Parsing of the SPIR-V to provide information for validation (core check, sync val, etc)
+    // (small performance hit for large shader)
+    bool spirv_parse = true;
+    // If we want to have spirv-opt potentially do constant folding on the spec constants
+    // (by far the largest performance bottle neck for large shaders using spec cosntants)
+    bool spirv_const_fold = true;
 };
 
 class DebugReport;

layers/state_tracker/pipeline_library_state.cpp

@@ -99,7 +99,8 @@ PreRasterState::PreRasterState(const vvl::Pipeline &p, const vvl::DeviceState &s
                 // don't need to worry about GroupDecoration in GPL
                 spirv::StatelessData *stateless_data_stage =
                     (stateless_data && i < kCommonMaxGraphicsShaderStages) ? &stateless_data[i] : nullptr;
-                auto spirv_module = std::make_shared<spirv::Module>(shader_ci->codeSize, shader_ci->pCode, stateless_data_stage);
+                auto spirv_module = vvl::CreateSpirvModuleState(shader_ci->codeSize, shader_ci->pCode, state_data.global_settings,
+                                                                stateless_data_stage);
                 module_state = std::make_shared<vvl::ShaderModule>(VK_NULL_HANDLE, spirv_module);
                 if (stateless_data_stage) {
                     stateless_data_stage->pipeline_pnext_module = spirv_module;
@@ -219,8 +220,8 @@ void SetFragmentShaderInfoPrivate(const vvl::Pipeline &pipeline_state, FragmentS
                     // don't need to worry about GroupDecoration in GPL
                     spirv::StatelessData *stateless_data_stage =
                         (stateless_data && i < kCommonMaxGraphicsShaderStages) ? &stateless_data[i] : nullptr;
-                    auto spirv_module =
-                        std::make_shared<spirv::Module>(shader_ci->codeSize, shader_ci->pCode, stateless_data_stage);
+                    auto spirv_module = vvl::CreateSpirvModuleState(shader_ci->codeSize, shader_ci->pCode,
+                                                                    state_data.global_settings, stateless_data_stage);
                     module_state = std::make_shared<vvl::ShaderModule>(VK_NULL_HANDLE, spirv_module);
                     if (stateless_data_stage) {
                         stateless_data_stage->pipeline_pnext_module = spirv_module;

layers/state_tracker/pipeline_state.cpp

@@ -91,7 +91,8 @@ std::vector<ShaderStageState> Pipeline::GetStageStates(const DeviceState &state_
             // This support was also added in VK_KHR_maintenance5
             if (const auto shader_ci = vku::FindStructInPNextChain<VkShaderModuleCreateInfo>(stage_ci.pNext)) {
                 // don't need to worry about GroupDecoration in GPL
-                auto spirv_module = std::make_shared<spirv::Module>(shader_ci->codeSize, shader_ci->pCode, stateless_data);
+                auto spirv_module =
+                    CreateSpirvModuleState(shader_ci->codeSize, shader_ci->pCode, state_data.global_settings, stateless_data);
                 module_state = std::make_shared<vvl::ShaderModule>(VK_NULL_HANDLE, spirv_module);
                 if (stateless_data) {
                     stateless_data->pipeline_pnext_module = spirv_module;

layers/state_tracker/shader_module.cpp

@@ -21,6 +21,7 @@
 #include <queue>
 
 #include <vulkan/utility/vk_format_utils.h>
+#include "layer_options.h"
 #include "utils/assert_utils.h"
 #include "utils/hash_util.h"
 #include "generated/spirv_grammar_helper.h"
@@ -944,8 +945,11 @@ EntryPoint::EntryPoint(const Module& module_state, const Instruction& entrypoint
     }
 }
 
-Module::StaticData::StaticData(const Module& module_state, StatelessData* stateless_data) {
-    if (!module_state.valid_spirv) return;
+Module::StaticData::StaticData(const Module& module_state, bool parse, StatelessData* stateless_data) {
+    // If parse is set off, save time because StaticData is never accessed
+    if (!module_state.valid_spirv || !parse) {
+        return;
+    }
 
     // Parse the words first so we have instruction class objects to use
     {
@@ -2601,3 +2605,15 @@ bool Module::UsesStorageCapabilityStorageClass(const Instruction& insn) const {
 }
 
 }  // namespace spirv
+
+namespace vvl {
+// Need to allow a way to not waste time copying over to spirv::Module::words_ when we don't want to store the SPIR-V
+std::shared_ptr<spirv::Module> CreateSpirvModuleState(size_t codeSize, const uint32_t* pCode, const GlobalSettings& global_settings,
+                                                      spirv::StatelessData* stateless_data) {
+    const bool is_valid_spirv = (pCode && pCode[0] == spv::MagicNumber && ((codeSize % 4) == 0));
+    if (!global_settings.spirv_store) {
+        return std::make_shared<spirv::Module>(is_valid_spirv);
+    }
+    return std::make_shared<spirv::Module>(codeSize, pCode, is_valid_spirv, global_settings.spirv_parse, stateless_data);
+}
+}  // namespace vvl

layers/state_tracker/shader_module.h

@@ -632,7 +632,7 @@ struct Module {
     // The goal of this struct is to move everything that is ready only into here
     struct StaticData {
         StaticData() = default;
-        StaticData(const Module &module_state, StatelessData *stateless_data = nullptr);
+        StaticData(const Module &module_state, bool parse, StatelessData *stateless_data);
         StaticData &operator=(StaticData &&) = default;
         StaticData(StaticData &&) = default;
 
@@ -713,14 +713,20 @@ struct Module {
     VulkanTypedHandle handle_;                            // Will be updated once its known its valid SPIR-V
     VulkanTypedHandle handle() const { return handle_; }  // matches normal convention to get handle
 
-    // Used for when modifying the SPIR-V (spirv-opt, GPU-AV instrumentation, etc) and need reparse it for VVL validation
-    Module(vvl::span<const uint32_t> code) : valid_spirv(true), words_(code.begin(), code.end()), static_data_(*this) {}
+    // Only currently used for when modifying the SPIR-V after spirv-opt and we need reparse it for VVL validation
+    explicit Module(vvl::span<const uint32_t> code)
+        : valid_spirv(true), words_(code.begin(), code.end()), static_data_(*this, true, nullptr) {}
 
+    // Used when we want to create a spirv::Module object (to make it easier to have a handle everywhere) but don't actually want to
+    // store/parse the SPIR-V itself (because it is turned off via settings)
+    explicit Module(bool is_valid_spirv) : valid_spirv(is_valid_spirv) {}
+
+    // "Normal" case
     // StatelessData is a pointer as we have cases were we don't need it and simpler to just null check the few cases that use it
-    Module(size_t codeSize, const uint32_t *pCode, StatelessData *stateless_data = nullptr)
-        : valid_spirv(pCode && pCode[0] == spv::MagicNumber && ((codeSize % 4) == 0)),
+    Module(size_t codeSize, const uint32_t *pCode, bool is_valid_spirv, bool parse, StatelessData *stateless_data)
+        : valid_spirv(is_valid_spirv),
           words_(pCode, pCode + codeSize / sizeof(uint32_t)),
-          static_data_(*this, stateless_data) {}
+          static_data_(*this, parse, stateless_data) {}
 
     const Instruction *FindDef(uint32_t id) const {
         auto it = static_data_.definitions.find(id);
@@ -789,14 +795,19 @@ struct Module {
 
 }  // namespace spirv
 
+struct GlobalSettings;
+
 // Represents a VkShaderModule handle
 namespace vvl {
+
+// Need to allow a way to not waste time copying over to spirv::Module::words_ when we don't want to store the SPIR-V
+std::shared_ptr<spirv::Module> CreateSpirvModuleState(size_t codeSize, const uint32_t *pCode, const GlobalSettings &global_settings,
+                                                      spirv::StatelessData *stateless_data = nullptr);
+
 struct ShaderModule : public StateObject {
     ShaderModule(VkShaderModule handle, std::shared_ptr<spirv::Module> &spirv_module)
         : StateObject(handle, kVulkanObjectTypeShaderModule), spirv(spirv_module) {
-        if (spirv) {
-            spirv->handle_ = handle_;
-        }
+        spirv->handle_ = handle_;
     }
 
     // For when we need to create a module with no SPIR-V backing it