[ir] Fix issues with InboundSiblingBranches. In each of the `continue`, break-if` and `next-iteration` instructions we have to remove the inbound sibling branches on `Destroy`. The `continue` instruction when you did a `SetLoop` was incorrectly using the `Body` branch of the loop instead of the `Continuing` branch. Change-Id: I4efe166180941154502fcfa75a51253d5858085f Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/244354 Commit-Queue: dan sinclair <dsinclair@chromium.org> Reviewed-by: James Price <jrprice@google.com>
diff --git a/src/tint/lang/core/ir/break_if.cc b/src/tint/lang/core/ir/break_if.cc index 0191868..9ab1be7 100644 --- a/src/tint/lang/core/ir/break_if.cc +++ b/src/tint/lang/core/ir/break_if.cc
@@ -62,6 +62,13 @@ BreakIf::~BreakIf() = default; +void BreakIf::Destroy() { + if (loop_) { + loop_->Body()->RemoveInboundSiblingBranch(this); + } + Instruction::Destroy(); +} + BreakIf* BreakIf::Clone(CloneContext& ctx) { auto* loop = ctx.Remap(loop_); auto* cond = ctx.Remap(Condition());
diff --git a/src/tint/lang/core/ir/break_if.h b/src/tint/lang/core/ir/break_if.h index 132ea20..a5be066 100644 --- a/src/tint/lang/core/ir/break_if.h +++ b/src/tint/lang/core/ir/break_if.h
@@ -71,6 +71,9 @@ ~BreakIf() override; + /// @copydoc Instruction::Destroy() + void Destroy() override; + /// @copydoc Instruction::Clone() BreakIf* Clone(CloneContext& ctx) override;
diff --git a/src/tint/lang/core/ir/break_if_test.cc b/src/tint/lang/core/ir/break_if_test.cc index b1c1cc7..be3e5a7 100644 --- a/src/tint/lang/core/ir/break_if_test.cc +++ b/src/tint/lang/core/ir/break_if_test.cc
@@ -129,5 +129,18 @@ EXPECT_THAT(loop2->Exits(), testing::ElementsAre(brk)); } +TEST_F(IR_BreakIfTest, Destroy) { + auto* loop = b.Loop(); + auto* cond = b.Constant(true); + auto* inst = b.BreakIf(loop, cond); + + ASSERT_EQ(1u, loop->Body()->InboundSiblingBranches().Length()); + EXPECT_EQ(inst, loop->Body()->InboundSiblingBranches()[0]); + + inst->Destroy(); + + EXPECT_EQ(0u, loop->Body()->InboundSiblingBranches().Length()); +} + } // namespace } // namespace tint::core::ir
diff --git a/src/tint/lang/core/ir/continue.cc b/src/tint/lang/core/ir/continue.cc index c36abe1..ae32299 100644 --- a/src/tint/lang/core/ir/continue.cc +++ b/src/tint/lang/core/ir/continue.cc
@@ -54,6 +54,13 @@ Continue::~Continue() = default; +void Continue::Destroy() { + if (loop_) { + loop_->Continuing()->RemoveInboundSiblingBranch(this); + } + Instruction::Destroy(); +} + Continue* Continue::Clone(CloneContext& ctx) { auto* loop = ctx.Remap(Loop()); auto args = ctx.Remap<Continue::kDefaultNumOperands>(Args()); @@ -63,11 +70,11 @@ void Continue::SetLoop(ir::Loop* loop) { if (loop_ && loop_->Body()) { - loop_->Body()->RemoveInboundSiblingBranch(this); + loop_->Continuing()->RemoveInboundSiblingBranch(this); } loop_ = loop; if (loop) { - loop->Body()->AddInboundSiblingBranch(this); + loop->Continuing()->AddInboundSiblingBranch(this); } }
diff --git a/src/tint/lang/core/ir/continue.h b/src/tint/lang/core/ir/continue.h index dab2080..a5eecd7 100644 --- a/src/tint/lang/core/ir/continue.h +++ b/src/tint/lang/core/ir/continue.h
@@ -61,6 +61,9 @@ /// @copydoc Instruction::Clone() Continue* Clone(CloneContext& ctx) override; + /// @copydoc Instruction::Destroy() + void Destroy() override; + /// @returns the loop owning the continue block ir::Loop* Loop() { return loop_; }
diff --git a/src/tint/lang/core/ir/continue_test.cc b/src/tint/lang/core/ir/continue_test.cc index 85a5e7e..a9f4518 100644 --- a/src/tint/lang/core/ir/continue_test.cc +++ b/src/tint/lang/core/ir/continue_test.cc
@@ -101,5 +101,17 @@ EXPECT_EQ(2_u, val1->As<core::constant::Scalar<u32>>()->ValueAs<u32>()); } +TEST_F(IR_ContinueTest, Destroy) { + auto* loop = b.Loop(); + auto* inst = b.Continue(loop); + + ASSERT_EQ(1u, loop->Continuing()->InboundSiblingBranches().Length()); + EXPECT_EQ(inst, loop->Continuing()->InboundSiblingBranches()[0]); + + inst->Destroy(); + + EXPECT_EQ(0u, loop->Continuing()->InboundSiblingBranches().Length()); +} + } // namespace } // namespace tint::core::ir
diff --git a/src/tint/lang/core/ir/next_iteration.cc b/src/tint/lang/core/ir/next_iteration.cc index bba8de8..425f505 100644 --- a/src/tint/lang/core/ir/next_iteration.cc +++ b/src/tint/lang/core/ir/next_iteration.cc
@@ -54,6 +54,13 @@ NextIteration::~NextIteration() = default; +void NextIteration::Destroy() { + if (loop_) { + loop_->Body()->RemoveInboundSiblingBranch(this); + } + Instruction::Destroy(); +} + NextIteration* NextIteration::Clone(CloneContext& ctx) { auto* new_loop = ctx.Clone(loop_); auto args = ctx.Remap<NextIteration::kDefaultNumOperands>(Args());
diff --git a/src/tint/lang/core/ir/next_iteration.h b/src/tint/lang/core/ir/next_iteration.h index 825f8c0..13e1723 100644 --- a/src/tint/lang/core/ir/next_iteration.h +++ b/src/tint/lang/core/ir/next_iteration.h
@@ -59,6 +59,9 @@ ~NextIteration() override; + /// @copydoc Instruction::Destroy() + void Destroy() override; + /// @copydoc Instruction::Clone() NextIteration* Clone(CloneContext& ctx) override;
diff --git a/src/tint/lang/core/ir/next_iteration_test.cc b/src/tint/lang/core/ir/next_iteration_test.cc index 6a7a6e3..b2cc9185 100644 --- a/src/tint/lang/core/ir/next_iteration_test.cc +++ b/src/tint/lang/core/ir/next_iteration_test.cc
@@ -84,5 +84,17 @@ EXPECT_EQ(new_loop, new_inst->Loop()); } +TEST_F(IR_NextIterationTest, Destroy) { + auto* loop = b.Loop(); + auto* inst = b.NextIteration(loop); + + ASSERT_EQ(1u, loop->Body()->InboundSiblingBranches().Length()); + EXPECT_EQ(inst, loop->Body()->InboundSiblingBranches()[0]); + + inst->Destroy(); + + EXPECT_EQ(0u, loop->Body()->InboundSiblingBranches().Length()); +} + } // namespace } // namespace tint::core::ir