[ir][validation] Walk through if/switch/loop in the validator Add support for walking through the blocks of if/switch and loop now that the structure has settled. Fixup a few tests along the way that are broken due to failed validation. This updates the disassembler to assume that an if always has a true block and a loop always has a body. This is then validated by the generic block code that a terminator is required in a block. Bug: tint:1952 Change-Id: I8e18e8b314b1b113602fa279143b96d870706690 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/139505 Reviewed-by: Ben Clayton <bclayton@google.com> Kokoro: Kokoro <noreply+kokoro@google.com> Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
diff --git a/src/tint/ir/disassembler.cc b/src/tint/ir/disassembler.cc index 38e9be4..5bb2201 100644 --- a/src/tint/ir/disassembler.cc +++ b/src/tint/ir/disassembler.cc
@@ -538,18 +538,11 @@ out_ << "if "; EmitOperand(if_, if_->Condition(), If::kConditionOperandOffset); - bool has_true = !if_->True()->IsEmpty(); bool has_false = !if_->False()->IsEmpty(); - out_ << " ["; - if (has_true) { - out_ << "t: %b" << IdOf(if_->True()); - } + out_ << " [t: %b" << IdOf(if_->True()); if (has_false) { - if (has_true) { - out_ << ", "; - } - out_ << "f: %b" << IdOf(if_->False()); + out_ << ", f: %b" << IdOf(if_->False()); } out_ << "]"; sm.Store(if_); @@ -557,10 +550,12 @@ out_ << " { # " << NameOf(if_); EmitLine(); - if (has_true) { + // True block is assumed to have instructions + { ScopedIndent si(indent_size_); EmitBlock(if_->True(), "true"); } + if (has_false) { ScopedIndent si(indent_size_); EmitBlock(if_->False(), "false"); @@ -584,9 +579,7 @@ if (!l->Initializer()->IsEmpty()) { parts.Push("i: %b" + std::to_string(IdOf(l->Initializer()))); } - if (!l->Body()->IsEmpty()) { - parts.Push("b: %b" + std::to_string(IdOf(l->Body()))); - } + parts.Push("b: %b" + std::to_string(IdOf(l->Body()))); if (!l->Continuing()->IsEmpty()) { parts.Push("c: %b" + std::to_string(IdOf(l->Continuing()))); @@ -603,7 +596,8 @@ EmitBlock(l->Initializer(), "initializer"); } - if (!l->Body()->IsEmpty()) { + // Loop is assumed to always have a body + { ScopedIndent si(indent_size_); EmitBlock(l->Body(), "body"); }
diff --git a/src/tint/ir/transform/merge_return_test.cc b/src/tint/ir/transform/merge_return_test.cc index 0991bb5..2e59642 100644 --- a/src/tint/ir/transform/merge_return_test.cc +++ b/src/tint/ir/transform/merge_return_test.cc
@@ -100,7 +100,8 @@ auto* swtch = b.Switch(in); b.With(b.Case(swtch, {Switch::CaseSelector{}}), [&] { b.ExitSwitch(swtch); }); - b.Loop(); + auto* l = b.Loop(); + b.With(l->Body(), [&] { b.ExitLoop(l); }); auto* ifelse = b.If(cond); ifelse->SetResults(b.InstructionResult(ty.i32())); @@ -118,14 +119,17 @@ exit_switch # switch_1 } } - loop [] { # loop_1 + loop [b: %b3] { # loop_1 + %b3 = block { # body + exit_loop # loop_1 + } } - %3:i32 = if %4 [t: %b3, f: %b4] { # if_1 - %b3 = block { # true + %3:i32 = if %4 [t: %b4, f: %b5] { # if_1 + %b4 = block { # true %5:i32 = add %2, 1i exit_if %5 # if_1 } - %b4 = block { # false + %b5 = block { # false %6:i32 = add %2, 2i exit_if %6 # if_1 } @@ -1506,7 +1510,8 @@ b.With(func->Block(), [&] { auto* sw = b.Switch(cond); b.With(b.Case(sw, {Switch::CaseSelector{b.Constant(1_i)}}), [&] { - auto* ifelse = b.If(cond); + auto* ifcond = b.Equal(ty.bool_(), cond, 1_i); + auto* ifelse = b.If(ifcond); b.With(ifelse->True(), [&] { b.Return(func, 42_i); }); b.With(ifelse->False(), [&] { b.ExitIf(ifelse); }); @@ -1528,7 +1533,8 @@ %b2 = block { switch %3 [c: (1i, %b3), c: (default, %b4)] { # switch_1 %b3 = block { # case - if %3 [t: %b5, f: %b6] { # if_1 + %4:bool = eq %3, 1i + if %4 [t: %b5, f: %b6] { # if_1 %b5 = block { # true ret 42i } @@ -1560,7 +1566,8 @@ %continue_execution:ptr<function, bool, read_write> = var, true switch %3 [c: (1i, %b3), c: (default, %b4)] { # switch_1 %b3 = block { # case - if %3 [t: %b5, f: %b6] { # if_1 + %6:bool = eq %3, 1i + if %6 [t: %b5, f: %b6] { # if_1 %b5 = block { # true store %continue_execution, false store %return_value, 42i @@ -1570,8 +1577,8 @@ exit_if # if_1 } } - %6:bool = load %continue_execution - if %6 [t: %b7] { # if_2 + %7:bool = load %continue_execution + if %7 [t: %b7] { # if_2 %b7 = block { # true store %1, 2i exit_switch # switch_1 @@ -1583,15 +1590,15 @@ exit_switch # switch_1 } } - %7:bool = load %continue_execution - if %7 [t: %b8] { # if_3 + %8:bool = load %continue_execution + if %8 [t: %b8] { # if_3 %b8 = block { # true store %return_value, 0i exit_if # if_3 } } - %8:i32 = load %return_value - ret %8 + %9:i32 = load %return_value + ret %9 } } )";
diff --git a/src/tint/ir/validate.cc b/src/tint/ir/validate.cc index a743786..e43a208 100644 --- a/src/tint/ir/validate.cc +++ b/src/tint/ir/validate.cc
@@ -35,6 +35,7 @@ #include "src/tint/ir/if.h" #include "src/tint/ir/load.h" #include "src/tint/ir/loop.h" +#include "src/tint/ir/multi_in_block.h" #include "src/tint/ir/next_iteration.h" #include "src/tint/ir/return.h" #include "src/tint/ir/store.h" @@ -266,9 +267,9 @@ [&](Call* c) { CheckCall(c); }, // [&](If* if_) { CheckIf(if_); }, // [&](Load*) {}, // - [&](Loop*) {}, // + [&](Loop* l) { CheckLoop(l); }, // [&](Store*) {}, // - [&](Switch*) {}, // + [&](Switch* s) { CheckSwitch(s); }, // [&](Swizzle*) {}, // [&](Terminator* b) { CheckTerminator(b); }, // [&](Unary* u) { CheckUnary(u); }, // @@ -405,6 +406,28 @@ if (if_->Condition() && !if_->Condition()->Type()->Is<type::Bool>()) { AddError(if_, If::kConditionOperandOffset, "if: condition must be a `bool` type"); } + + CheckBlock(if_->True()); + if (!if_->False()->IsEmpty()) { + CheckBlock(if_->False()); + } + } + + void CheckLoop(Loop* l) { + if (!l->Initializer()->IsEmpty()) { + CheckBlock(l->Initializer()); + } + CheckBlock(l->Body()); + + if (!l->Continuing()->IsEmpty()) { + CheckBlock(l->Continuing()); + } + } + + void CheckSwitch(Switch* s) { + for (auto& cse : s->Cases()) { + CheckBlock(cse.block); + } } void CheckVar(Var* var) {
diff --git a/src/tint/ir/validate_test.cc b/src/tint/ir/validate_test.cc index 723c3fc..db8079f 100644 --- a/src/tint/ir/validate_test.cc +++ b/src/tint/ir/validate_test.cc
@@ -515,6 +515,50 @@ )"); } +TEST_F(IR_ValidateTest, If_EmptyFalse) { + auto* f = b.Function("my_func", ty.void_()); + + auto* if_ = b.If(true); + if_->True()->Append(b.Return(f)); + + f->Block()->Append(if_); + f->Block()->Append(b.Return(f)); + + auto res = ir::Validate(mod); + EXPECT_TRUE(res) << res.Failure().str(); +} + +TEST_F(IR_ValidateTest, If_EmptyTrue) { + auto* f = b.Function("my_func", ty.void_()); + + auto* if_ = b.If(true); + if_->False()->Append(b.Return(f)); + + f->Block()->Append(if_); + f->Block()->Append(b.Return(f)); + + auto res = ir::Validate(mod); + ASSERT_FALSE(res); + EXPECT_EQ(res.Failure().str(), R"(:4:7 error: block: does not end in a terminator instruction + %b2 = block { # true + ^^^^^^^^^^^ + +note: # Disassembly +%my_func = func():void -> %b1 { + %b1 = block { + if true [t: %b2, f: %b3] { # if_1 + %b2 = block { # true + } + %b3 = block { # false + ret + } + } + ret + } +} +)"); +} + TEST_F(IR_ValidateTest, If_ConditionIsBool) { auto* f = b.Function("my_func", ty.void_()); @@ -589,6 +633,46 @@ )"); } +TEST_F(IR_ValidateTest, Loop_OnlyBody) { + auto* f = b.Function("my_func", ty.void_()); + + auto* l = b.Loop(); + l->Body()->Append(b.ExitLoop(l)); + + auto sb = b.With(f->Block()); + sb.Append(l); + sb.Return(f); + + auto res = ir::Validate(mod); + EXPECT_TRUE(res) << res.Failure().str(); +} + +TEST_F(IR_ValidateTest, Loop_EmptyBody) { + auto* f = b.Function("my_func", ty.void_()); + + auto sb = b.With(f->Block()); + sb.Append(b.Loop()); + sb.Return(f); + + auto res = ir::Validate(mod); + ASSERT_FALSE(res); + EXPECT_EQ(res.Failure().str(), R"(:4:7 error: block: does not end in a terminator instruction + %b2 = block { # body + ^^^^^^^^^^^ + +note: # Disassembly +%my_func = func():void -> %b1 { + %b1 = block { + loop [b: %b2] { # loop_1 + %b2 = block { # body + } + } + ret + } +} +)"); +} + TEST_F(IR_ValidateTest, Var_RootBlock_NullResult) { auto* v = mod.instructions.Create<ir::Var>(nullptr); b.RootBlock()->Append(v);