From 2c458414c0851b9a0d1b0493857b56a1089847ac Mon Sep 17 00:00:00 2001 From: David Neto Date: Wed, 25 Nov 2020 14:54:32 -0500 Subject: [PATCH] BuildModule: optionally avoid adding new OpLine instructions (#4033) * BuildModule: optionally avoid adding new OpLine instructions Fixes #4029 for my use case * Fix formatting * Create last_line_inst_ only if doing extra line tracking --- source/opt/build_module.cpp | 9 ++++ source/opt/build_module.h | 10 +++- source/opt/ir_loader.cpp | 3 +- source/opt/ir_loader.h | 12 ++++- test/opt/ir_loader_test.cpp | 98 ++++++++++++++++++++++++++++++++++++- 5 files changed, 128 insertions(+), 4 deletions(-) diff --git a/source/opt/build_module.cpp b/source/opt/build_module.cpp index fc76a3c29d..3b606dc2b5 100644 --- a/source/opt/build_module.cpp +++ b/source/opt/build_module.cpp @@ -50,11 +50,20 @@ std::unique_ptr BuildModule(spv_target_env env, MessageConsumer consumer, const uint32_t* binary, const size_t size) { + return BuildModule(env, consumer, binary, size, true); +} + +std::unique_ptr BuildModule(spv_target_env env, + MessageConsumer consumer, + const uint32_t* binary, + const size_t size, + bool extra_line_tracking) { auto context = spvContextCreate(env); SetContextMessageConsumer(context, consumer); auto irContext = MakeUnique(env, consumer); opt::IrLoader loader(consumer, irContext->module()); + loader.SetExtraLineTracking(extra_line_tracking); spv_result_t status = spvBinaryParse(context, &loader, binary, size, SetSpvHeader, SetSpvInst, nullptr); diff --git a/source/opt/build_module.h b/source/opt/build_module.h index c9d1cf2e45..29eaf66139 100644 --- a/source/opt/build_module.h +++ b/source/opt/build_module.h @@ -27,7 +27,15 @@ namespace spvtools { // Builds an Module returns the owning IRContext from the given SPIR-V // |binary|. |size| specifies number of words in |binary|. The |binary| will be // decoded according to the given target |env|. Returns nullptr if errors occur -// and sends the errors to |consumer|. +// and sends the errors to |consumer|. When |extra_line_tracking| is true, +// extra OpLine instructions are injected to better presere line numbers while +// later transforms mutate the module. +std::unique_ptr BuildModule(spv_target_env env, + MessageConsumer consumer, + const uint32_t* binary, size_t size, + bool extra_line_tracking); + +// Like above, with extra line tracking turned on. std::unique_ptr BuildModule(spv_target_env env, MessageConsumer consumer, const uint32_t* binary, diff --git a/source/opt/ir_loader.cpp b/source/opt/ir_loader.cpp index 4a44309444..06099ce066 100644 --- a/source/opt/ir_loader.cpp +++ b/source/opt/ir_loader.cpp @@ -92,7 +92,8 @@ bool IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) { std::unique_ptr spv_inst( new Instruction(module()->context(), *inst, std::move(dbg_line_info_))); if (!spv_inst->dbg_line_insts().empty()) { - if (spv_inst->dbg_line_insts().back().opcode() != SpvOpNoLine) { + if (extra_line_tracking_ && + (spv_inst->dbg_line_insts().back().opcode() != SpvOpNoLine)) { last_line_inst_ = std::unique_ptr( spv_inst->dbg_line_insts().back().Clone(module()->context())); } diff --git a/source/opt/ir_loader.h b/source/opt/ir_loader.h index d0610f139e..16bc2c7c84 100644 --- a/source/opt/ir_loader.h +++ b/source/opt/ir_loader.h @@ -63,6 +63,10 @@ class IrLoader { // or a missing OpFunctionEnd. Resolves internal bookkeeping. void EndModule(); + // Sets whether extra OpLine instructions should be injected to better + // track line information. + void SetExtraLineTracking(bool flag) { extra_line_tracking_ = flag; } + private: // Consumer for communicating messages to outside. const MessageConsumer& consumer_; @@ -78,11 +82,17 @@ class IrLoader { std::unique_ptr block_; // Line related debug instructions accumulated thus far. std::vector dbg_line_info_; - // Line instruction that should be applied to the next instruction. + // If doing extra line tracking, this is the line instruction that should be + // applied to the next instruction. Otherwise it always contains null. std::unique_ptr last_line_inst_; // The last DebugScope information that IrLoader::AddInstruction() handled. DebugScope last_dbg_scope_; + + // When true, do extra line information tracking: Additional OpLine + // instructions will be injected to help track line info more robustly during + // transformations. + bool extra_line_tracking_ = true; }; } // namespace opt diff --git a/test/opt/ir_loader_test.cpp b/test/opt/ir_loader_test.cpp index 8b77aa33b0..475dd2351e 100644 --- a/test/opt/ir_loader_test.cpp +++ b/test/opt/ir_loader_test.cpp @@ -19,7 +19,7 @@ #include #include -#include "gtest/gtest.h" +#include "gmock/gmock.h" #include "source/opt/build_module.h" #include "source/opt/def_use_manager.h" #include "source/opt/ir_context.h" @@ -29,6 +29,8 @@ namespace spvtools { namespace opt { namespace { +using ::testing::ContainerEq; + constexpr uint32_t kOpLineOperandLineIndex = 1; void DoRoundTripCheck(const std::string& text) { @@ -236,6 +238,100 @@ TEST(IrBuilder, DistributeLineDebugInfo) { } } +TEST(IrBuilder, BuildModule_WithoutExtraLines) { + const std::string text = R"(OpCapability Shader +OpMemoryModel Logical Simple +OpEntryPoint Vertex %main "main" +%file = OpString "my file" +%void = OpTypeVoid +%voidfn = OpTypeFunction %void +%float = OpTypeFloat 32 +%float_1 = OpConstant %float 1 +%main = OpFunction %void None %voidfn +%100 = OpLabel +%1 = OpFAdd %float %float_1 %float_1 +OpLine %file 1 0 +%2 = OpFMul %float %1 %1 +%3 = OpFSub %float %2 %2 +OpReturn +OpFunctionEnd +)"; + + std::vector binary; + SpirvTools t(SPV_ENV_UNIVERSAL_1_1); + ASSERT_TRUE(t.Assemble(text, &binary, + SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS)); + + // This is the function we're testing. + std::unique_ptr context = BuildModule( + SPV_ENV_UNIVERSAL_1_5, nullptr, binary.data(), binary.size(), false); + ASSERT_NE(nullptr, context); + + spvtools::opt::analysis::DefUseManager* def_use_mgr = + context->get_def_use_mgr(); + + std::vector opcodes; + for (auto* inst = def_use_mgr->GetDef(1); + inst && (inst->opcode() != SpvOpFunctionEnd); inst = inst->NextNode()) { + inst->ForEachInst( + [&opcodes](spvtools::opt::Instruction* sub_inst) { + opcodes.push_back(sub_inst->opcode()); + }, + true); + } + + EXPECT_THAT(opcodes, + ContainerEq(std::vector{SpvOpFAdd, SpvOpLine, SpvOpFMul, + SpvOpFSub, SpvOpReturn})); +} + +TEST(IrBuilder, BuildModule_WithExtraLines_IsDefault) { + const std::string text = R"(OpCapability Shader +OpMemoryModel Logical Simple +OpEntryPoint Vertex %main "main" +%file = OpString "my file" +%void = OpTypeVoid +%voidfn = OpTypeFunction %void +%float = OpTypeFloat 32 +%float_1 = OpConstant %float 1 +%main = OpFunction %void None %voidfn +%100 = OpLabel +%1 = OpFAdd %float %float_1 %float_1 +OpLine %file 1 0 +%2 = OpFMul %float %1 %1 +%3 = OpFSub %float %2 %2 +OpReturn +OpFunctionEnd +)"; + + std::vector binary; + + SpirvTools t(SPV_ENV_UNIVERSAL_1_1); + ASSERT_TRUE(t.Assemble(text, &binary, + SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS)); + + // This is the function we're testing. + std::unique_ptr context = + BuildModule(SPV_ENV_UNIVERSAL_1_5, nullptr, binary.data(), binary.size()); + + spvtools::opt::analysis::DefUseManager* def_use_mgr = + context->get_def_use_mgr(); + + std::vector opcodes; + for (auto* inst = def_use_mgr->GetDef(1); + inst && (inst->opcode() != SpvOpFunctionEnd); inst = inst->NextNode()) { + inst->ForEachInst( + [&opcodes](spvtools::opt::Instruction* sub_inst) { + opcodes.push_back(sub_inst->opcode()); + }, + true); + } + + EXPECT_THAT(opcodes, ContainerEq(std::vector{ + SpvOpFAdd, SpvOpLine, SpvOpFMul, SpvOpLine, + SpvOpFSub, SpvOpLine, SpvOpReturn})); +} + TEST(IrBuilder, ConsumeDebugInfoInst) { // /* HLSL */ //