Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BuildModule: optionally avoid adding new OpLine instructions #4033

Merged
merged 3 commits into from
Nov 25, 2020
Merged

BuildModule: optionally avoid adding new OpLine instructions #4033

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a259717
BuildModule: optionally avoid adding new OpLine instructions
dneto0 Nov 24, 2020
b608026
Fix formatting
dneto0 Nov 25, 2020
acf87e1
Create last_line_inst_ only if doing extra line tracking
dneto0 Nov 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions source/opt/build_module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,20 @@ std::unique_ptr<opt::IRContext> 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<opt::IRContext> 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<opt::IRContext>(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);
Expand Down
10 changes: 9 additions & 1 deletion source/opt/build_module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<opt::IRContext> 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<opt::IRContext> BuildModule(spv_target_env env,
MessageConsumer consumer,
const uint32_t* binary,
Expand Down
4 changes: 3 additions & 1 deletion source/opt/ir_loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ bool IrLoader::AddInstruction(const spv_parsed_instruction_t* inst) {
dbg_line_info_.clear();
} else if (last_line_inst_ != nullptr) {
last_line_inst_->SetDebugScope(last_dbg_scope_);
spv_inst->dbg_line_insts().push_back(*last_line_inst_);
if (extra_line_tracking_) {
Copy link
Contributor

@jaebaek jaebaek Nov 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issues.

We do not need the line 95 to the line 98 if extra_line_tracking_ is false.
The following code will let us skip cloning last_line_inst_:

    if (extra_line_tracking_ && spv_inst->dbg_line_insts().back().opcode() != SpvOpNoLine) {
      last_line_inst_ = std::unique_ptr<Instruction>(
          spv_inst->dbg_line_insts().back().Clone(module()->context()));
    }

Since the above change will make last_line_inst_ always null when extra_line_tracking_ is false, we do not need additional if statement in the line 102.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I also updated the comment on last_line_inst_ in the header because now it's always null when extra_line_tracking_ is false.

spv_inst->dbg_line_insts().push_back(*last_line_inst_);
}
}

const char* src = source_.c_str();
Expand Down
8 changes: 8 additions & 0 deletions source/opt/ir_loader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand All @@ -83,6 +87,10 @@ class IrLoader {

// The last DebugScope information that IrLoader::AddInstruction() handled.
DebugScope last_dbg_scope_;

// When true, extra OpLine instructions are injected to help track line
// info more robustly during transformations.
bool extra_line_tracking_ = true;
};

} // namespace opt
Expand Down
98 changes: 97 additions & 1 deletion test/opt/ir_loader_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#include <utility>
#include <vector>

#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"
Expand All @@ -29,6 +29,8 @@ namespace spvtools {
namespace opt {
namespace {

using ::testing::ContainerEq;

constexpr uint32_t kOpLineOperandLineIndex = 1;

void DoRoundTripCheck(const std::string& text) {
Expand Down Expand Up @@ -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<uint32_t> 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<IRContext> 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<SpvOp> 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<SpvOp>{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<uint32_t> 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<IRContext> 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<SpvOp> 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<SpvOp>{
SpvOpFAdd, SpvOpLine, SpvOpFMul, SpvOpLine,
SpvOpFSub, SpvOpLine, SpvOpReturn}));
}

TEST(IrBuilder, ConsumeDebugInfoInst) {
// /* HLSL */
//
Expand Down