From 106b7a6e4af7fadf78c39132087b06026584e919 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 4 Apr 2017 17:57:24 -0700 Subject: [PATCH 01/10] tests: Add test to ensure spec constant default values are used Currently we don't even look at these, and instead assume `1`, which is completely bogus. --- tests/layer_validation_tests.cpp | 48 ++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index 9573e9d01f..b8608ea075 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -14697,6 +14697,54 @@ TEST_F(VkLayerTest, CreatePipelineMissingEntrypoint) { m_errorMonitor->VerifyFound(); } +TEST_F(VkLayerTest, CreatePipelineVsFsArraySizeDefaultSpecialization) { + m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_ERROR_BIT_EXT, "Type mismatch"); + m_errorMonitor->SetUnexpectedError("consumes input location 1.0 which is not written"); + + // Trigger a type mismatch when default specialization constant values are + // not used. At time of writing, SC doesn't even look at the default values, and + // so instead treats everything as `1`. + + ASSERT_NO_FATAL_FAILURE(Init()); + ASSERT_NO_FATAL_FAILURE(InitRenderTarget()); + + char const *vsSource = + "#version 450\n" + "out gl_PerVertex {\n" + " vec4 gl_Position;\n" + "};\n" + "layout(constant_id=0) const uint size = 2;\n" + "layout(location=0) out float xs[size];\n" + "void main(){\n" + " gl_Position = vec4(0);\n" + " xs[0] = 42.0;\n" + "}\n"; + char const *fsSource = + "#version 450\n" + "\n" + "layout(location=0) in float xs[1];\n" + "layout(location=0) out vec4 color;\n" + "void main(){\n" + " color = vec4(xs[0]);\n" + "}\n"; + + VkShaderObj vs(m_device, vsSource, VK_SHADER_STAGE_VERTEX_BIT, this); + VkShaderObj fs(m_device, fsSource, VK_SHADER_STAGE_FRAGMENT_BIT, this); + + VkPipelineObj pipe(m_device); + pipe.AddColorAttachment(); + pipe.AddShader(&vs); + pipe.AddShader(&fs); + + VkDescriptorSetObj descriptorSet(m_device); + descriptorSet.AppendDummy(); + descriptorSet.CreateVKDescriptorSet(m_commandBuffer); + + pipe.CreateVKPipeline(descriptorSet.GetPipelineLayout(), renderPass()); + + m_errorMonitor->VerifyFound(); +} + TEST_F(VkLayerTest, CreatePipelineDepthStencilRequired) { m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_ERROR_BIT_EXT, "pDepthStencilState is NULL when rasterization is enabled and subpass " From 5426d5e8bab894b409459038c808506adc515266 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 4 Apr 2017 17:57:58 -0700 Subject: [PATCH 02/10] tests: Add test for a specialization-induced array size mismatch These shaders are fine without the specialization -- but with it applied, we should get a type mismatch on the array sizes. --- tests/layer_validation_tests.cpp | 53 ++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp index b8608ea075..48b269aeef 100644 --- a/tests/layer_validation_tests.cpp +++ b/tests/layer_validation_tests.cpp @@ -14745,6 +14745,59 @@ TEST_F(VkLayerTest, CreatePipelineVsFsArraySizeDefaultSpecialization) { m_errorMonitor->VerifyFound(); } +TEST_F(VkLayerTest, CreatePipelineVsFsArraySizeSpecialization) { + m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_ERROR_BIT_EXT, "Type mismatch"); + + // Trigger a type mismatch when the VS output array size is specialized. If + // specializations are not applied, then these interfaces match fine. + + ASSERT_NO_FATAL_FAILURE(Init()); + ASSERT_NO_FATAL_FAILURE(InitRenderTarget()); + + char const *vsSource = + "#version 450\n" + "out gl_PerVertex {\n" + " vec4 gl_Position;\n" + "};\n" + "layout(constant_id=0) const uint size = 1;\n" + "layout(location=0) out float xs[size];\n" + "void main(){\n" + " gl_Position = vec4(0);\n" + " for (int i = 0; i < size; i++) { xs[i] = 42.0; }\n" + "}\n"; + char const *fsSource = + "#version 450\n" + "\n" + "layout(location=0) in float xs[1];\n" + "layout(location=0) out vec4 color;\n" + "void main(){\n" + " color = vec4(xs[0]);\n" + "}\n"; + + VkShaderObj vs(m_device, vsSource, VK_SHADER_STAGE_VERTEX_BIT, this); + VkShaderObj fs(m_device, fsSource, VK_SHADER_STAGE_FRAGMENT_BIT, this); + + // Specialize the VS + uint32_t actual_size = 2; + VkSpecializationMapEntry spec_entry = { 0, 0, sizeof(actual_size) }; + VkSpecializationInfo spec = { 1, &spec_entry, sizeof(actual_size), &actual_size }; + auto vs_stage = vs.GetStageCreateInfo(); + vs_stage.pSpecializationInfo = &spec; + + VkPipelineObj pipe(m_device); + pipe.AddColorAttachment(); + pipe.AddShader(vs_stage); + pipe.AddShader(&fs); + + VkDescriptorSetObj descriptorSet(m_device); + descriptorSet.AppendDummy(); + descriptorSet.CreateVKDescriptorSet(m_commandBuffer); + + pipe.CreateVKPipeline(descriptorSet.GetPipelineLayout(), renderPass()); + + m_errorMonitor->VerifyFound(); +} + TEST_F(VkLayerTest, CreatePipelineDepthStencilRequired) { m_errorMonitor->SetDesiredFailureMsg(VK_DEBUG_REPORT_ERROR_BIT_EXT, "pDepthStencilState is NULL when rasterization is enabled and subpass " From 3b6318137d9cd8232c88349df31ddb80fe9aa657 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Wed, 5 Apr 2017 15:54:33 -0700 Subject: [PATCH 03/10] layers: Adjust spirv-tools includes in CV --- layers/core_validation.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index ded7b803c6..274e288e0f 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -66,7 +66,8 @@ #include "vk_layer_data.h" #include "vk_layer_extension_utils.h" #include "vk_layer_utils.h" -#include "spirv-tools/libspirv.h" +#include "spirv-tools/libspirv.hpp" +#include "spirv-tools/optimizer.hpp" #if defined __ANDROID__ #include From df3ae18747c6a97db8d871e06d9a1990bbb3aada Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 11 Apr 2017 15:34:47 -0700 Subject: [PATCH 04/10] layers: Add helper for getting VkShaderModule state struct The SC code was previously reaching directly into the layer_data, which we'd rather not do. Also removes some iterator clutter from SC. --- layers/core_validation.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 274e288e0f..2ac1367bea 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -407,6 +407,14 @@ SURFACE_STATE *GetSurfaceState(instance_layer_data *instance_data, VkSurfaceKHR return &it->second; } +struct shader_module *GetShaderState(layer_data *dev_data, VkShaderModule shader_module) { + auto it = dev_data->shaderModuleMap.find(shader_module); + if (it == dev_data->shaderModuleMap.end()) { + return nullptr; + } + return &it->second; +} + // Return ptr to memory binding for given handle of specified type static BINDABLE *GetObjectMemBinding(layer_data *dev_data, uint64_t handle, VkDebugReportObjectTypeEXT type) { switch (type) { @@ -2611,8 +2619,7 @@ static bool validate_pipeline_shader_stage( layer_data *dev_data, VkPipelineShaderStageCreateInfo const *pStage, PIPELINE_STATE *pipeline, shader_module **out_module, spirv_inst_iter *out_entrypoint) { bool pass = true; - auto module_it = dev_data->shaderModuleMap.find(pStage->module); - auto module = *out_module = module_it->second.get(); + auto module = *out_module = GetShaderState(dev_data, pStage->module); auto report_data = dev_data->report_data; if (!module->has_valid_spirv) return pass; From 6c1450173d5bf47a2091c29c2b577fc65747074d Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 11 Apr 2017 15:39:25 -0700 Subject: [PATCH 05/10] layers: Remove defunct TODO from SC This was done long ago. --- layers/core_validation.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 2ac1367bea..7ea4fbc44d 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -1250,8 +1250,6 @@ static unsigned get_locations_consumed_by_type(shader_module const *src, unsigne default: // Everything else is just 1. return 1; - - // TODO: extend to handle 64bit scalar types, whose vectors may need multiple locations. } } From b741602d8179ab8b18fb656d2ea44052a83c74b9 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 11 Apr 2017 15:43:39 -0700 Subject: [PATCH 06/10] layers: Validate VkSpecializationInfo much earlier We're about to actually apply specializations, so let's do this work /before/ broken specializations will cause us to crash. --- layers/core_validation.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 7ea4fbc44d..cec4954686 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -2620,6 +2620,8 @@ static bool validate_pipeline_shader_stage( auto module = *out_module = GetShaderState(dev_data, pStage->module); auto report_data = dev_data->report_data; + pass &= validate_specialization_offsets(report_data, pStage); + if (!module->has_valid_spirv) return pass; // Find the entrypoint @@ -2643,7 +2645,6 @@ static bool validate_pipeline_shader_stage( auto pipelineLayout = pipeline->pipeline_layout; - pass &= validate_specialization_offsets(report_data, pStage); pass &= validate_push_constant_usage(report_data, &pipelineLayout.push_constant_ranges, module, accessible_ids, pStage->stage); // Validate descriptor use From 735f22bd8cabd08b6278472c82b51419af4ef13b Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 11 Apr 2017 16:16:37 -0700 Subject: [PATCH 07/10] fixup --- layers/core_validation.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index cec4954686..56e7a5c781 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -412,7 +412,7 @@ struct shader_module *GetShaderState(layer_data *dev_data, VkShaderModule shader if (it == dev_data->shaderModuleMap.end()) { return nullptr; } - return &it->second; + return it->second.get(); } // Return ptr to memory binding for given handle of specified type From 3322561e7786dccff2cba86926e7de58a5e6edf2 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 11 Apr 2017 16:16:43 -0700 Subject: [PATCH 08/10] NOMERGE: spirv-tools optimizer use --- layers/core_validation.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp index 56e7a5c781..4b24b0f076 100644 --- a/layers/core_validation.cpp +++ b/layers/core_validation.cpp @@ -2624,6 +2624,32 @@ static bool validate_pipeline_shader_stage( if (!module->has_valid_spirv) return pass; + spvtools::Optimizer opt(SPV_ENV_VULKAN_1_0); + + if (pStage->pSpecializationInfo) { + // massage the specialization data blob into the form spirv-tools wants + std::unordered_map specializations; + for (auto i = 0u; i < pStage->pSpecializationInfo->mapEntryCount; i++) { + auto const & mapEntry = pStage->pSpecializationInfo->pMapEntries[i]; + specializations[mapEntry.constantID] = std::string( + reinterpret_cast(pStage->pSpecializationInfo->pData) + mapEntry.offset, + mapEntry.size); + } + opt.RegisterPass(spvtools::CreateSetSpecConstantDefaultValuePass(specializations)); + } + + opt.RegisterPass(spvtools::CreateFreezeSpecConstantValuePass()); + opt.RegisterPass(spvtools::CreateUnifyConstantPass()); + + std::vector final_spirv; + if (!opt.Run(module->words.data(), module->words.size(), &final_spirv)) { + // TODO: route any spirv-tools optimizer diagnostics here + if (log_msg(dev_data->report_data, VK_DEBUG_REPORT_ERROR_BIT_EXT, VkDebugReportObjectTypeEXT(0), 0, __LINE__, SHADER_CHECKER_INCONSISTENT_SPIRV, + "SC", "Internal error preprocessing shader module")) { + return false; + } + } + // Find the entrypoint auto entrypoint = *out_entrypoint = find_entrypoint(module, pStage->pName, pStage->stage); if (entrypoint == module->end()) { From 82750b1f6389f5a3a28d1e82c5f04fbbb0e69429 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 11 Apr 2017 16:30:56 -0700 Subject: [PATCH 09/10] cmake noise --- CMakeLists.txt | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a7b8a89f3c..d99e16ef59 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -208,6 +208,9 @@ find_library(SPIRV_REMAPPER_LIB NAMES SPVRemapper find_library(SPIRV_TOOLS_LIB NAMES SPIRV-Tools HINTS ${SPIRV_TOOLS_SEARCH_PATH} ) +find_library(SPIRV_TOOLS_OPT_LIB NAMES SPIRV-Tools-opt + HINTS ${SPIRV_TOOLS_SEARCH_PATH}/opt ) + if (WIN32) add_library(glslang STATIC IMPORTED) add_library(OGLCompiler STATIC IMPORTED) @@ -217,6 +220,7 @@ if (WIN32) add_library(SPVRemapper STATIC IMPORTED) add_library(Loader STATIC IMPORTED) add_library(SPIRV-Tools STATIC IMPORTED) + add_library(SPIRV-Tools-opt STATIC IMPORTED) find_library(GLSLANG_DLIB NAMES glslangd HINTS ${GLSLANG_DEBUG_SEARCH_PATH} ) @@ -232,6 +236,8 @@ if (WIN32) HINTS ${GLSLANG_DEBUG_SEARCH_PATH} ) find_library(SPIRV_TOOLS_DLIB NAMES SPIRV-Tools HINTS ${SPIRV_TOOLS_DEBUG_SEARCH_PATH} ) + find_library(SPIRV_TOOLS_OPT_DLIB NAMES SPIRV-Tools-opt + HINTS ${SPIRV_TOOLS_DEBUG_SEARCH_PATH}/opt ) set_target_properties(glslang PROPERTIES IMPORTED_LOCATION "${GLSLANG_LIB}" @@ -254,12 +260,15 @@ if (WIN32) set_target_properties(SPIRV-Tools PROPERTIES IMPORTED_LOCATION "${SPIRV_TOOLS_LIB}" IMPORTED_LOCATION_DEBUG "${SPIRV_TOOLS_DLIB}") + set_target_properties(SPIRV-Tools-opt PROPERTIES + IMPORTED_LOCATION "${SPIRV_TOOLS_OPT_LIB}" + IMPORTED_LOCATION_DEBUG "${SPIRV_TOOLS_OPT_DLIB}") set (GLSLANG_LIBRARIES glslang OGLCompiler OSDependent HLSL SPIRV SPVRemapper) - set (SPIRV_TOOLS_LIBRARIES SPIRV-Tools) + set (SPIRV_TOOLS_LIBRARIES SPIRV-Tools SPIRV-Tools-opt) else () set (GLSLANG_LIBRARIES ${GLSLANG_LIB} ${OGLCompiler_LIB} ${OSDependent_LIB} ${HLSL_LIB} ${SPIRV_LIB} ${SPIRV_REMAPPER_LIB}) - set (SPIRV_TOOLS_LIBRARIES ${SPIRV_TOOLS_LIB}) + set (SPIRV_TOOLS_LIBRARIES ${SPIRV_TOOLS_LIB} ${SPIRV_TOOLS_OPT_LIB}) endif() set (PYTHON_CMD ${PYTHON_EXECUTABLE}) From a6d2c013e714329c34c93ada1237c9fba6374c08 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Tue, 11 Apr 2017 16:54:53 -0700 Subject: [PATCH 10/10] cmake noise; get lib order right so opt finds the symbols it needs in tools --- CMakeLists.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d99e16ef59..d34547002f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -265,10 +265,10 @@ if (WIN32) IMPORTED_LOCATION_DEBUG "${SPIRV_TOOLS_OPT_DLIB}") set (GLSLANG_LIBRARIES glslang OGLCompiler OSDependent HLSL SPIRV SPVRemapper) - set (SPIRV_TOOLS_LIBRARIES SPIRV-Tools SPIRV-Tools-opt) + set (SPIRV_TOOLS_LIBRARIES SPIRV-Tools-opt SPIRV-Tools) else () set (GLSLANG_LIBRARIES ${GLSLANG_LIB} ${OGLCompiler_LIB} ${OSDependent_LIB} ${HLSL_LIB} ${SPIRV_LIB} ${SPIRV_REMAPPER_LIB}) - set (SPIRV_TOOLS_LIBRARIES ${SPIRV_TOOLS_LIB} ${SPIRV_TOOLS_OPT_LIB}) + set (SPIRV_TOOLS_LIBRARIES ${SPIRV_TOOLS_OPT_LIB} ${SPIRV_TOOLS_LIB}) endif() set (PYTHON_CMD ${PYTHON_EXECUTABLE})