From e3992f240821992499bb5ce7ffe9e5fdcee7392f Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Sat, 7 Jan 2023 23:15:47 +0000 Subject: [PATCH] [ir] Cleanup disassembler output This CL expands the disassembler output and makes a bit more useful. The case selector value is changed to an `ir::Constant` instead of the `constant::Value` to make disassembly easier. The `BuilderImpl` is updated to not fail in the face of missing implementation but continue as best as possible. Bug: tint:1718 Change-Id: I8b275a19bccbb02bb785d311778198bb0c9e0456 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/116547 Reviewed-by: Ben Clayton Kokoro: Kokoro Commit-Queue: Dan Sinclair --- src/tint/ir/builder_impl.cc | 15 +++++++-- src/tint/ir/builder_impl_test.cc | 20 ++++++----- src/tint/ir/disassembler.cc | 58 +++++++++++++++++++++----------- src/tint/ir/switch.h | 4 +-- 4 files changed, 64 insertions(+), 33 deletions(-) diff --git a/src/tint/ir/builder_impl.cc b/src/tint/ir/builder_impl.cc index dbcad5efa4..ed4a156b0d 100644 --- a/src/tint/ir/builder_impl.cc +++ b/src/tint/ir/builder_impl.cc @@ -45,6 +45,7 @@ #include "src/tint/sem/expression.h" #include "src/tint/sem/module.h" #include "src/tint/sem/switch_statement.h" +#include "src/tint/type/void.h" namespace tint::ir { namespace { @@ -237,6 +238,8 @@ bool BuilderImpl::EmitStatement(const ast::Statement* stmt) { diagnostics_.add_warning( tint::diag::System::IR, "unknown statement type: " + std::string(stmt->TypeInfo().name), stmt->source); + // TODO(dsinclair): This should return `false`, switch back when all + // the cases are handled. return true; }); } @@ -461,7 +464,7 @@ bool BuilderImpl::EmitSwitch(const ast::SwitchStatement* stmt) { if (selector->IsDefault()) { selectors.Push({nullptr}); } else { - selectors.Push({selector->Value()->Clone(clone_ctx_)}); + selectors.Push({builder.Constant(selector->Value()->Clone(clone_ctx_))}); } } @@ -575,7 +578,10 @@ utils::Result BuilderImpl::EmitExpression(const ast::Expression* expr) { diagnostics_.add_warning( tint::diag::System::IR, "unknown expression type: " + std::string(expr->TypeInfo().name), expr->source); - return utils::Failure; + // TODO(dsinclair): This should return utils::Failure; Switch back + // once all the above cases are handled. + auto* v = builder.ir.types.Get(); + return builder.Temp(v); }); } @@ -590,7 +596,10 @@ bool BuilderImpl::EmitVariable(const ast::Variable* var) { diagnostics_.add_warning(tint::diag::System::IR, "unknown variable: " + std::string(var->TypeInfo().name), var->source); - return false; + + // TODO(dsinclair): This should return `false`, switch back when all + // the cases are handled. + return true; }); } diff --git a/src/tint/ir/builder_impl_test.cc b/src/tint/ir/builder_impl_test.cc index eb3811ebd5..bc3a23ec5b 100644 --- a/src/tint/ir/builder_impl_test.cc +++ b/src/tint/ir/builder_impl_test.cc @@ -1125,12 +1125,14 @@ TEST_F(IR_BuilderImplTest, Switch) { auto* func = m.functions[0]; ASSERT_EQ(1u, flow->cases[0].selectors.Length()); - ASSERT_TRUE(flow->cases[0].selectors[0].val->Is>()); - EXPECT_EQ(0_i, flow->cases[0].selectors[0].val->As>()->ValueOf()); + ASSERT_TRUE(flow->cases[0].selectors[0].val->value->Is>()); + EXPECT_EQ(0_i, + flow->cases[0].selectors[0].val->value->As>()->ValueOf()); ASSERT_EQ(1u, flow->cases[1].selectors.Length()); - ASSERT_TRUE(flow->cases[1].selectors[0].val->Is>()); - EXPECT_EQ(1_i, flow->cases[1].selectors[0].val->As>()->ValueOf()); + ASSERT_TRUE(flow->cases[1].selectors[0].val->value->Is>()); + EXPECT_EQ(1_i, + flow->cases[1].selectors[0].val->value->As>()->ValueOf()); ASSERT_EQ(1u, flow->cases[2].selectors.Length()); EXPECT_TRUE(flow->cases[2].selectors[0].IsDefault()); @@ -1227,8 +1229,9 @@ TEST_F(IR_BuilderImplTest, Switch_WithBreak) { auto* func = m.functions[0]; ASSERT_EQ(1u, flow->cases[0].selectors.Length()); - ASSERT_TRUE(flow->cases[0].selectors[0].val->Is>()); - EXPECT_EQ(0_i, flow->cases[0].selectors[0].val->As>()->ValueOf()); + ASSERT_TRUE(flow->cases[0].selectors[0].val->value->Is>()); + EXPECT_EQ(0_i, + flow->cases[0].selectors[0].val->value->As>()->ValueOf()); ASSERT_EQ(1u, flow->cases[1].selectors.Length()); EXPECT_TRUE(flow->cases[1].selectors[0].IsDefault()); @@ -1291,8 +1294,9 @@ TEST_F(IR_BuilderImplTest, Switch_AllReturn) { auto* func = m.functions[0]; ASSERT_EQ(1u, flow->cases[0].selectors.Length()); - ASSERT_TRUE(flow->cases[0].selectors[0].val->Is>()); - EXPECT_EQ(0_i, flow->cases[0].selectors[0].val->As>()->ValueOf()); + ASSERT_TRUE(flow->cases[0].selectors[0].val->value->Is>()); + EXPECT_EQ(0_i, + flow->cases[0].selectors[0].val->value->As>()->ValueOf()); ASSERT_EQ(1u, flow->cases[1].selectors.Length()); EXPECT_TRUE(flow->cases[1].selectors[0].IsDefault()); diff --git a/src/tint/ir/disassembler.cc b/src/tint/ir/disassembler.cc index e65bccafcf..d3201c03c0 100644 --- a/src/tint/ir/disassembler.cc +++ b/src/tint/ir/disassembler.cc @@ -62,6 +62,7 @@ std::ostream& Disassembler::Indent() { void Disassembler::EmitBlockInstructions(const Block* b) { for (const auto* instr : b->instructions) { + Indent(); instr->ToString(out_, mod_.symbols) << std::endl; } } @@ -85,7 +86,8 @@ void Disassembler::Walk(const FlowNode* node) { tint::Switch( node, [&](const ir::Function* f) { - Indent() << "%" << GetIdForNode(f) << " = Function" << std::endl; + Indent() << "%bb" << GetIdForNode(f) << " = Function " << mod_.symbols.NameFor(f->name) + << std::endl; { ScopedIndent func_indent(&indent_size_); @@ -95,76 +97,92 @@ void Disassembler::Walk(const FlowNode* node) { Walk(f->end_target); }, [&](const ir::Block* b) { - Indent() << "%" << GetIdForNode(b) << " = Block" << std::endl; + Indent() << "%bb" << GetIdForNode(b) << " = Block" << std::endl; EmitBlockInstructions(b); if (b->branch.target->Is()) { - Indent() << "Return "; + Indent() << "Return"; } else { - Indent() << "Branch "; + Indent() << "BranchTo " + << "%bb" << GetIdForNode(b->branch.target); } - out_ << GetIdForNode(b->branch.target); - + out_ << " ("; for (const auto* v : b->branch.args) { - out_ << " "; + if (v != b->branch.args.Front()) { + out_ << ", "; + } v->ToString(out_, mod_.symbols); } - out_ << std::endl; + out_ << ")" << std::endl << std::endl; Walk(b->branch.target); }, [&](const ir::Switch* s) { - Indent() << "%" << GetIdForNode(s) << " = Switch (" << s->condition << ")" << std::endl; + Indent() << "%bb" << GetIdForNode(s) << " = Switch ("; + s->condition->ToString(out_, mod_.symbols); + out_ << ")" << std::endl; { ScopedIndent switch_indent(&indent_size_); ScopedStopNode scope(&stop_nodes_, s->merge.target); for (const auto& c : s->cases) { - Indent() << "Case" << std::endl; + Indent() << "# Case "; + for (const auto& selector : c.selectors) { + if (selector.IsDefault()) { + out_ << "default "; + } else { + selector.val->ToString(out_, mod_.symbols); + } + } + out_ << std::endl; ScopedIndent case_indent(&indent_size_); Walk(c.start.target); } } - Indent() << "Switch Merge" << std::endl; + Indent() << "# Switch Merge" << std::endl; Walk(s->merge.target); }, [&](const ir::If* i) { - Indent() << "%" << GetIdForNode(i) << " = if (" << i->condition << ")" << std::endl; + Indent() << "%bb" << GetIdForNode(i) << " = if ("; + i->condition->ToString(out_, mod_.symbols); + out_ << ")" << std::endl; + { ScopedIndent if_indent(&indent_size_); ScopedStopNode scope(&stop_nodes_, i->merge.target); - Indent() << "true branch" << std::endl; + Indent() << "# true branch" << std::endl; Walk(i->true_.target); - Indent() << "false branch" << std::endl; + Indent() << "# false branch" << std::endl; Walk(i->false_.target); } - Indent() << "if merge" << std::endl; + Indent() << "# if merge" << std::endl; Walk(i->merge.target); }, [&](const ir::Loop* l) { - Indent() << "%" << GetIdForNode(l) << " = loop" << std::endl; + Indent() << "%bb" << GetIdForNode(l) << " = loop" << std::endl; { ScopedStopNode loop_scope(&stop_nodes_, l->merge.target); ScopedIndent loop_indent(&indent_size_); { ScopedStopNode inner_scope(&stop_nodes_, l->continuing.target); - Indent() << "loop start" << std::endl; + Indent() << "# loop start" << std::endl; Walk(l->start.target); } - Indent() << "loop continuing" << std::endl; + Indent() << "# loop continuing" << std::endl; ScopedIndent continuing_indent(&indent_size_); Walk(l->continuing.target); } - Indent() << "loop merge" << std::endl; + Indent() << "# loop merge" << std::endl; Walk(l->merge.target); }, - [&](const ir::Terminator*) { Indent() << "Function end" << std::endl; }); + [&](const ir::Terminator*) { Indent() << "FunctionEnd" << std::endl + << std::endl; }); } std::string Disassembler::Disassemble() { diff --git a/src/tint/ir/switch.h b/src/tint/ir/switch.h index 6d3a952ce4..6ba1d4cfab 100644 --- a/src/tint/ir/switch.h +++ b/src/tint/ir/switch.h @@ -15,9 +15,9 @@ #ifndef SRC_TINT_IR_SWITCH_H_ #define SRC_TINT_IR_SWITCH_H_ -#include "src/tint/constant/value.h" #include "src/tint/ir/block.h" #include "src/tint/ir/branch.h" +#include "src/tint/ir/constant.h" #include "src/tint/ir/flow_node.h" #include "src/tint/ir/value.h" @@ -32,7 +32,7 @@ class Switch : public Castable { bool IsDefault() const { return val == nullptr; } /// The selector value, or nullptr if this is the default selector - constant::Value* val = nullptr; + Constant* val = nullptr; }; /// A case label in the struct