From 3855447d93eb9a79723e0711b895b57ae444ce3f Mon Sep 17 00:00:00 2001 From: greg-lunarg Date: Mon, 22 Jul 2019 11:51:39 -0600 Subject: [PATCH] Bindless Instrument: Make init check depend solely on input_init_enabled (#2753) * Bindless Instrument: Make init check depend solely on input_init_enabled Previously was dependent on presense of descriptor_indexing extension in SPIR-V, but this missed some cases. Tests updated to refect this new policy. * Fix format. --- source/opt/inst_bindless_check_pass.cpp | 19 ++------ source/opt/inst_bindless_check_pass.h | 3 -- test/opt/inst_bindless_check_test.cpp | 58 +++++++++++++------------ 3 files changed, 34 insertions(+), 46 deletions(-) diff --git a/source/opt/inst_bindless_check_pass.cpp b/source/opt/inst_bindless_check_pass.cpp index 2eeab46e1..b283354b0 100644 --- a/source/opt/inst_bindless_check_pass.cpp +++ b/source/opt/inst_bindless_check_pass.cpp @@ -398,20 +398,9 @@ void InstBindlessCheckPass::GenInitCheckCode( void InstBindlessCheckPass::InitializeInstBindlessCheck() { // Initialize base class InitializeInstrument(); - // Look for related extensions - ext_descriptor_indexing_defined_ = false; - for (auto& ei : get_module()->extensions()) { - const char* ext_name = - reinterpret_cast(&ei.GetInOperand(0).words[0]); - if (strcmp(ext_name, "SPV_EXT_descriptor_indexing") == 0) { - ext_descriptor_indexing_defined_ = true; - break; - } - } - // If descriptor indexing extension and runtime array length support enabled, - // create variable mappings. Length support is always enabled if descriptor - // init check is enabled. - if (ext_descriptor_indexing_defined_ && input_length_enabled_) + // If runtime array length support enabled, create variable mappings. Length + // support is always enabled if descriptor init check is enabled. + if (input_length_enabled_) for (auto& anno : get_module()->annotations()) if (anno.opcode() == SpvOpDecorate) { if (anno.GetSingleWordInOperand(1u) == SpvDecorationDescriptorSet) @@ -433,7 +422,7 @@ Pass::Status InstBindlessCheckPass::ProcessImpl() { new_blocks); }; bool modified = InstProcessEntryPointCallTree(pfn); - if (ext_descriptor_indexing_defined_ && input_init_enabled_) { + if (input_init_enabled_) { // Perform descriptor initialization check on each entry point function in // module pfn = [this](BasicBlock::iterator ref_inst_itr, diff --git a/source/opt/inst_bindless_check_pass.h b/source/opt/inst_bindless_check_pass.h index 12384b1fd..51d771277 100644 --- a/source/opt/inst_bindless_check_pass.h +++ b/source/opt/inst_bindless_check_pass.h @@ -162,9 +162,6 @@ class InstBindlessCheckPass : public InstrumentPass { // GenInitCheckCode to every instruction in module. Pass::Status ProcessImpl(); - // True if VK_EXT_descriptor_indexing is defined - bool ext_descriptor_indexing_defined_; - // Enable instrumentation of runtime array length checking bool input_length_enabled_; diff --git a/test/opt/inst_bindless_check_test.cpp b/test/opt/inst_bindless_check_test.cpp index 25f0baa15..6fe27c64a 100644 --- a/test/opt/inst_bindless_check_test.cpp +++ b/test/opt/inst_bindless_check_test.cpp @@ -255,7 +255,7 @@ OpFunctionEnd func_pt2_before, entry_after + names_annots + new_annots + consts_types_vars + new_consts_types_vars + func_pt1 + func_pt2_after + output_func, - true, true); + true, true, 7u, 23u, false, false, 1u); } TEST_F(InstBindlessTest, NoInstrumentConstIndexInbounds) { @@ -335,7 +335,8 @@ OpFunctionEnd )"; SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); - SinglePassRunAndCheck(before, before, true, true); + SinglePassRunAndCheck(before, before, true, true, 7u, + 23u, false, false, 1u); } TEST_F(InstBindlessTest, InstrumentMultipleInstructions) { @@ -630,7 +631,7 @@ OpFunctionEnd SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true); + true, 7u, 23u, false, false, 1u); } TEST_F(InstBindlessTest, InstrumentOpImage) { @@ -858,7 +859,7 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true); + true, 7u, 23u, false, false, 1u); } TEST_F(InstBindlessTest, InstrumentSampledImage) { @@ -1081,7 +1082,7 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true); + true, 7u, 23u, false, false, 1u); } TEST_F(InstBindlessTest, InstrumentImageWrite) { @@ -1306,7 +1307,7 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true); + true, 7u, 23u, false, false, 1u); } TEST_F(InstBindlessTest, InstrumentVertexSimple) { @@ -1580,7 +1581,7 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true); + true, 7u, 23u, false, false, 1u); } TEST_F(InstBindlessTest, MultipleDebugFunctions) { @@ -1903,7 +1904,8 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func1_before + func2_before, - defs_after + func1_after + func2_after + output_func, true, true); + defs_after + func1_after + func2_after + output_func, true, true, 7u, 23u, + false, false, 1u); } TEST_F(InstBindlessTest, RuntimeArray) { @@ -2276,18 +2278,19 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck(whole_file, whole_file, true, - true); + true, 7u, 23u, false, false, 1u); } TEST_F(InstBindlessTest, InstrumentInitCheckOnScalarDescriptor) { // This test verifies that the pass will correctly instrument vanilla // texture sample on a scalar descriptor with an initialization check if the - // SPV_EXT_descriptor_checking extension is enabled. This is the same shader - // as NoInstrumentNonBindless, but with the extension hacked on in the SPIR-V. + // input_init_enable argument is set to true. This can happen when the + // descriptor indexing extension is enabled in the API but the SPIR-V + // does not have the extension enabled because it does not contain a + // runtime array. This is the same shader as NoInstrumentNonBindless. const std::string defs_before = R"(OpCapability Shader -OpExtension "SPV_EXT_descriptor_indexing" %1 = OpExtInstImport "GLSL.std.450" OpMemoryModel Logical GLSL450 OpEntryPoint Fragment %MainPs "MainPs" %i_vTextureCoords %_entryPointOutput_vColor @@ -2324,7 +2327,6 @@ OpDecorate %_entryPointOutput_vColor Location 0 const std::string defs_after = R"(OpCapability Shader -OpExtension "SPV_EXT_descriptor_indexing" OpExtension "SPV_KHR_storage_buffer_storage_class" %1 = OpExtInstImport "GLSL.std.450" OpMemoryModel Logical GLSL450 @@ -2395,7 +2397,7 @@ OpDecorate %gl_FragCoord BuiltIn FragCoord %uint_6 = OpConstant %uint 6 %uint_7 = OpConstant %uint 7 %uint_8 = OpConstant %uint 8 -%uint_40 = OpConstant %uint 40 +%uint_39 = OpConstant %uint 39 %113 = OpConstantNull %v4float )"; @@ -2429,7 +2431,7 @@ OpBranchConditional %52 %55 %56 %59 = OpImageSampleImplicitLod %v4float %58 %20 OpBranch %54 %56 = OpLabel -%112 = OpFunctionCall %void %60 %uint_40 %uint_1 %uint_0 %uint_0 +%112 = OpFunctionCall %void %60 %uint_39 %uint_1 %uint_0 %uint_0 OpBranch %54 %54 = OpLabel %114 = OpPhi %v4float %59 %55 %113 %56 @@ -4456,7 +4458,7 @@ OpFunctionEnd func_pt2_before, entry_after + names_annots + new_annots + consts_types_vars + new_consts_types_vars + func_pt1 + func_pt2_after + output_func, - true, true, 7u, 23u, true, true, 2u); + true, true, 7u, 23u, false, false, 2u); } TEST_F(InstBindlessTest, InstrumentMultipleInstructionsV2) { @@ -4751,7 +4753,7 @@ OpFunctionEnd SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true, 7u, 23u, true, true, 2u); + true, 7u, 23u, false, false, 2u); } TEST_F(InstBindlessTest, InstrumentOpImageV2) { @@ -4979,7 +4981,7 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true, 7u, 23u, true, true, 2u); + true, 7u, 23u, false, false, 2u); } TEST_F(InstBindlessTest, InstrumentSampledImageV2) { @@ -5202,7 +5204,7 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true, 7u, 23u, true, true, 2u); + true, 7u, 23u, false, false, 2u); } TEST_F(InstBindlessTest, InstrumentImageWriteV2) { @@ -5427,7 +5429,7 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true, 7u, 23u, true, true, 2u); + true, 7u, 23u, false, false, 2u); } TEST_F(InstBindlessTest, InstrumentVertexSimpleV2) { @@ -5701,7 +5703,7 @@ OpFunctionEnd // SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); SinglePassRunAndCheck( defs_before + func_before, defs_after + func_after + output_func, true, - true, 7u, 23u, true, true, 2u); + true, 7u, 23u, false, false, 2u); } TEST_F(InstBindlessTest, MultipleDebugFunctionsV2) { @@ -6025,7 +6027,7 @@ OpFunctionEnd SinglePassRunAndCheck( defs_before + func1_before + func2_before, defs_after + func1_after + func2_after + output_func, true, true, 7u, 23u, - true, true, 2u); + false, false, 2u); } TEST_F(InstBindlessTest, RuntimeArrayV2) { @@ -6327,12 +6329,13 @@ OpFunctionEnd TEST_F(InstBindlessTest, InstrumentInitCheckOnScalarDescriptorV2) { // This test verifies that the pass will correctly instrument vanilla // texture sample on a scalar descriptor with an initialization check if the - // SPV_EXT_descriptor_checking extension is enabled. This is the same shader - // as NoInstrumentNonBindless, but with the extension hacked on in the SPIR-V. + // input_init_enable argument is set to true. This can happen when the + // descriptor indexing extension is enabled in the API but the SPIR-V + // does not have the extension enabled because it does not contain a + // runtime array. This is the same shader as NoInstrumentNonBindless. const std::string defs_before = R"(OpCapability Shader -OpExtension "SPV_EXT_descriptor_indexing" %1 = OpExtInstImport "GLSL.std.450" OpMemoryModel Logical GLSL450 OpEntryPoint Fragment %MainPs "MainPs" %i_vTextureCoords %_entryPointOutput_vColor @@ -6369,7 +6372,6 @@ OpDecorate %_entryPointOutput_vColor Location 0 const std::string defs_after = R"(OpCapability Shader -OpExtension "SPV_EXT_descriptor_indexing" OpExtension "SPV_KHR_storage_buffer_storage_class" %1 = OpExtInstImport "GLSL.std.450" OpMemoryModel Logical GLSL450 @@ -6440,7 +6442,7 @@ OpDecorate %gl_FragCoord BuiltIn FragCoord %uint_7 = OpConstant %uint 7 %uint_8 = OpConstant %uint 8 %uint_9 = OpConstant %uint 9 -%uint_40 = OpConstant %uint 40 +%uint_39 = OpConstant %uint 39 %113 = OpConstantNull %v4float )"; @@ -6474,7 +6476,7 @@ OpBranchConditional %52 %55 %56 %59 = OpImageSampleImplicitLod %v4float %58 %20 OpBranch %54 %56 = OpLabel -%112 = OpFunctionCall %void %60 %uint_40 %uint_1 %uint_0 %uint_0 +%112 = OpFunctionCall %void %60 %uint_39 %uint_1 %uint_0 %uint_0 OpBranch %54 %54 = OpLabel %114 = OpPhi %v4float %59 %55 %113 %56