From 8ca0aa710d4f91dae64de889696d9dcbd51f4d86 Mon Sep 17 00:00:00 2001 From: David Neto Date: Thu, 11 Jun 2020 18:11:15 +0000 Subject: [PATCH] [spirv-reader] Simplify if-selection bookkeeping In BlockInfo, remove the backpointers from true-head, false-head, and premerge-head to the if-selection header block. Convert the forward references from if-selection to its internal heads from pointers to IDs. Bug: tint:3 Change-Id: Ic931df519795e14374bff4f60ad37a4b32f79c91 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/23140 Reviewed-by: dan sinclair --- src/reader/spirv/function.cc | 19 ++-- src/reader/spirv/function.h | 34 ++---- src/reader/spirv/function_cfg_test.cc | 150 ++++++++++++-------------- 3 files changed, 89 insertions(+), 114 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index bf9ce5f7c5..3c42772054 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -1336,12 +1336,10 @@ bool FunctionEmitter::FindIfSelectionInternalHeaders() { const bool contains_false = construct->ContainsPos(false_head_pos); if (contains_true) { - true_head_info->true_head_for = construct.get(); - if_header_info->true_head = true_head_info; + if_header_info->true_head = true_head; } if (contains_false) { - false_head_info->false_head_for = construct.get(); - if_header_info->false_head = false_head_info; + if_header_info->false_head = false_head; } if ((true_head_info->header_for_merge != 0) && @@ -1438,8 +1436,7 @@ bool FunctionEmitter::FindIfSelectionInternalHeaders() { } premerge_id = dest_id; auto* dest_block_info = GetBlockInfo(dest_id); - dest_block_info->premerge_head_for = construct.get(); - if_header_info->premerge_head = dest_block_info; + if_header_info->premerge_head = dest_id; if (dest_block_info->header_for_merge != 0) { // Premerge has two edges coming into it, from the then-clause // and the else-clause. It's also, by construction, not the @@ -1743,8 +1740,8 @@ bool FunctionEmitter::EmitIfStart(const BlockInfo& block_info) { assert(construct->kind == Construct::kIfSelection); assert(construct->begin_id == block_info.id); - const auto* const false_head = block_info.false_head; - const auto* const premerge_head = block_info.premerge_head; + const uint32_t false_head = block_info.false_head; + const uint32_t premerge_head = block_info.premerge_head; auto* const if_stmt = AddStatement(std::make_unique())->AsIf(); @@ -1772,7 +1769,7 @@ bool FunctionEmitter::EmitIfStart(const BlockInfo& block_info) { // Emit it as: if (cond) {} else {....} // Move the merge up to where the premerge is. const uint32_t intended_merge = - premerge_head ? premerge_head->id : construct->end_id; + premerge_head ? premerge_head : construct->end_id; // then-clause: // If true_head exists: @@ -1781,11 +1778,11 @@ bool FunctionEmitter::EmitIfStart(const BlockInfo& block_info) { // Otherwise: // ends at from the false head (if it exists), otherwise the selection // end. - const uint32_t then_end = false_head ? false_head->id : intended_merge; + const uint32_t then_end = false_head ? false_head : intended_merge; // else-clause: // ends at the premerge head (if it exists) or at the selection end. - const uint32_t else_end = premerge_head ? premerge_head->id : intended_merge; + const uint32_t else_end = premerge_head ? premerge_head : intended_merge; // Push statement blocks for the then-clause and the else-clause. // But make sure we do it in the right order. diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index 30fe2526b9..a96f7e8cb6 100644 --- a/src/reader/spirv/function.h +++ b/src/reader/spirv/function.h @@ -137,31 +137,17 @@ struct BlockInfo { /// The following fields record relationships among blocks in a selection /// construct for an OpBranchConditional instruction. - /// If not null, then the pointed-at construct is a selection for an - /// OpBranchConditional and this block is the "true" target for it. We say - /// this block "heads" the true case. - const Construct* true_head_for = nullptr; - /// If not null, then the pointed-at construct is a selection for an - /// OpBranchConditional and this block is the "false" target for it. We say - /// this block "heads" the false case. - const Construct* false_head_for = nullptr; - /// If not null, then the pointed-at construct is the first block at which - /// control reconverges between the "then" and "else" clauses, but before - /// the merge block for that selection. - const Construct* premerge_head_for = nullptr; - /// If not null, then this block is an if-selection header, and |true_head| is - /// the target of the true branch on the OpBranchConditional. - /// In particular, true_head->true_head_for == this - const BlockInfo* true_head = nullptr; - /// If not null, then this block is an if-selection header, and |false_head| - /// is the target of the false branch on the OpBranchConditional. - /// In particular, false_head->false_head_for == this - const BlockInfo* false_head = nullptr; - /// If not null, then this block is an if-selection header, and when following + /// If not 0, then this block is an if-selection header, and |true_head| is + /// the target id of the true branch on the OpBranchConditional. + uint32_t true_head = 0; + /// If not 0, then this block is an if-selection header, and |false_head| + /// is the target id of the false branch on the OpBranchConditional. + uint32_t false_head = 0; + /// If not 0, then this block is an if-selection header, and when following /// the flow via the true and false branches, control first reconverges at - /// |premerge_head|, and |premerge_head| is still inside the if-selection. - /// In particular, premerge_head->premerge_head_for == this - const BlockInfo* premerge_head = nullptr; + /// the block with ID |premerge_head|, and |premerge_head| is still inside + /// the if-selection. + uint32_t premerge_head = 0; }; inline std::ostream& operator<<(std::ostream& o, const BlockInfo& bi) { diff --git a/src/reader/spirv/function_cfg_test.cc b/src/reader/spirv/function_cfg_test.cc index 3984f3bde1..4d64864987 100644 --- a/src/reader/spirv/function_cfg_test.cc +++ b/src/reader/spirv/function_cfg_test.cc @@ -6683,10 +6683,9 @@ TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_NoIf) { auto* bi = fe.GetBlockInfo(10); ASSERT_NE(bi, nullptr); - EXPECT_EQ(bi->true_head_for, nullptr); - EXPECT_EQ(bi->false_head_for, nullptr); - EXPECT_EQ(bi->premerge_head_for, nullptr); - EXPECT_EQ(bi->premerge_head_for, nullptr); + EXPECT_EQ(bi->true_head, 0u); + EXPECT_EQ(bi->false_head, 0u); + EXPECT_EQ(bi->premerge_head, 0u); } TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_ThenElse) { @@ -6711,19 +6710,29 @@ TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_ThenElse) { FunctionEmitter fe(p, *spirv_function(100)); EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + auto* bi10 = fe.GetBlockInfo(10); + ASSERT_NE(bi10, nullptr); + EXPECT_EQ(bi10->true_head, 20u); + EXPECT_EQ(bi10->false_head, 30u); + EXPECT_EQ(bi10->premerge_head, 0u); + auto* bi20 = fe.GetBlockInfo(20); ASSERT_NE(bi20, nullptr); - ASSERT_NE(bi20->true_head_for, nullptr); - EXPECT_EQ(bi20->true_head_for->begin_id, 10u); - EXPECT_EQ(bi20->false_head_for, nullptr); - EXPECT_EQ(bi20->premerge_head_for, nullptr); + EXPECT_EQ(bi20->true_head, 0u); + EXPECT_EQ(bi20->false_head, 0u); + EXPECT_EQ(bi20->premerge_head, 0u); auto* bi30 = fe.GetBlockInfo(30); ASSERT_NE(bi30, nullptr); - EXPECT_EQ(bi30->true_head_for, nullptr); - ASSERT_NE(bi30->false_head_for, nullptr); - EXPECT_EQ(bi30->false_head_for->begin_id, 10u); - EXPECT_EQ(bi30->premerge_head_for, nullptr); + EXPECT_EQ(bi30->true_head, 0u); + EXPECT_EQ(bi30->false_head, 0u); + EXPECT_EQ(bi30->premerge_head, 0u); + + auto* bi99 = fe.GetBlockInfo(99); + ASSERT_NE(bi99, nullptr); + EXPECT_EQ(bi99->true_head, 0u); + EXPECT_EQ(bi99->false_head, 0u); + EXPECT_EQ(bi99->premerge_head, 0u); } TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_IfOnly) { @@ -6745,12 +6754,23 @@ TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_IfOnly) { FunctionEmitter fe(p, *spirv_function(100)); EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + auto* bi10 = fe.GetBlockInfo(10); + ASSERT_NE(bi10, nullptr); + EXPECT_EQ(bi10->true_head, 30u); + EXPECT_EQ(bi10->false_head, 0u); + EXPECT_EQ(bi10->premerge_head, 0u); + auto* bi30 = fe.GetBlockInfo(30); ASSERT_NE(bi30, nullptr); - ASSERT_NE(bi30->true_head_for, nullptr); - EXPECT_EQ(bi30->true_head_for->begin_id, 10u); - EXPECT_EQ(bi30->false_head_for, nullptr); - EXPECT_EQ(bi30->premerge_head_for, nullptr); + EXPECT_EQ(bi30->true_head, 0u); + EXPECT_EQ(bi30->false_head, 0u); + EXPECT_EQ(bi30->premerge_head, 0u); + + auto* bi99 = fe.GetBlockInfo(99); + ASSERT_NE(bi99, nullptr); + EXPECT_EQ(bi99->true_head, 0u); + EXPECT_EQ(bi99->false_head, 0u); + EXPECT_EQ(bi99->premerge_head, 0u); } TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_ElseOnly) { @@ -6772,12 +6792,23 @@ TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_ElseOnly) { FunctionEmitter fe(p, *spirv_function(100)); EXPECT_TRUE(FlowFindIfSelectionInternalHeaders(&fe)); + auto* bi10 = fe.GetBlockInfo(10); + ASSERT_NE(bi10, nullptr); + EXPECT_EQ(bi10->true_head, 0u); + EXPECT_EQ(bi10->false_head, 30u); + EXPECT_EQ(bi10->premerge_head, 0u); + auto* bi30 = fe.GetBlockInfo(30); ASSERT_NE(bi30, nullptr); - EXPECT_EQ(bi30->true_head_for, nullptr); - ASSERT_NE(bi30->false_head_for, nullptr); - EXPECT_EQ(bi30->false_head_for->begin_id, 10u); - EXPECT_EQ(bi30->premerge_head_for, nullptr); + EXPECT_EQ(bi30->true_head, 0u); + EXPECT_EQ(bi30->false_head, 0u); + EXPECT_EQ(bi30->premerge_head, 0u); + + auto* bi99 = fe.GetBlockInfo(99); + ASSERT_NE(bi99, nullptr); + EXPECT_EQ(bi99->true_head, 0u); + EXPECT_EQ(bi99->false_head, 0u); + EXPECT_EQ(bi99->premerge_head, 0u); } TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_Regardless) { @@ -6804,19 +6835,11 @@ TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_Regardless) { EXPECT_THAT(fe.block_order(), ElementsAre(10, 20, 80, 99)); - auto* bi20 = fe.GetBlockInfo(20); - ASSERT_NE(bi20, nullptr); - ASSERT_NE(bi20->true_head_for, nullptr); - EXPECT_EQ(bi20->true_head_for->begin_id, 10u); - ASSERT_NE(bi20->false_head_for, nullptr); - EXPECT_EQ(bi20->false_head_for->begin_id, 10u); - EXPECT_EQ(bi20->premerge_head_for, nullptr); - - auto* bi80 = fe.GetBlockInfo(80); - ASSERT_NE(bi80, nullptr); - EXPECT_EQ(bi80->true_head_for, nullptr); - EXPECT_EQ(bi80->false_head_for, nullptr); - EXPECT_EQ(bi80->premerge_head_for, nullptr); + auto* bi10 = fe.GetBlockInfo(10); + ASSERT_NE(bi10, nullptr); + EXPECT_EQ(bi10->true_head, 20u); + EXPECT_EQ(bi10->false_head, 20u); + EXPECT_EQ(bi10->premerge_head, 0u); } TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_Premerge_Simple) { @@ -6846,12 +6869,11 @@ TEST_F(SpvParserTest, FindIfSelectionInternalHeaders_Premerge_Simple) { EXPECT_THAT(fe.block_order(), ElementsAre(10, 20, 30, 80, 99)); - auto* bi80 = fe.GetBlockInfo(80); - ASSERT_NE(bi80, nullptr); - EXPECT_EQ(bi80->true_head_for, nullptr); - EXPECT_EQ(bi80->false_head_for, nullptr); - ASSERT_NE(bi80->premerge_head_for, nullptr); - EXPECT_EQ(bi80->premerge_head_for->begin_id, 10u); + auto* bi10 = fe.GetBlockInfo(10); + ASSERT_NE(bi10, nullptr); + EXPECT_EQ(bi10->true_head, 20u); + EXPECT_EQ(bi10->false_head, 30u); + EXPECT_EQ(bi10->premerge_head, 80u); } TEST_F(SpvParserTest, @@ -6882,26 +6904,11 @@ TEST_F(SpvParserTest, EXPECT_THAT(fe.block_order(), ElementsAre(10, 20, 30, 80, 99)); - auto* bi20 = fe.GetBlockInfo(20); - ASSERT_NE(bi20, nullptr); - ASSERT_NE(bi20->true_head_for, nullptr); - EXPECT_EQ(bi20->true_head_for->begin_id, 10u); - EXPECT_EQ(bi20->false_head_for, nullptr); - EXPECT_EQ(bi20->premerge_head_for, nullptr); - - auto* bi30 = fe.GetBlockInfo(30); - ASSERT_NE(bi30, nullptr); - EXPECT_EQ(bi30->true_head_for, nullptr); - ASSERT_NE(bi30->false_head_for, nullptr); - EXPECT_EQ(bi30->false_head_for->begin_id, 10u); - ASSERT_NE(bi30->premerge_head_for, nullptr); - EXPECT_EQ(bi30->premerge_head_for->begin_id, 10u); - - auto* bi80 = fe.GetBlockInfo(80); - ASSERT_NE(bi80, nullptr); - EXPECT_EQ(bi80->true_head_for, nullptr); - EXPECT_EQ(bi80->false_head_for, nullptr); - EXPECT_EQ(bi80->premerge_head_for, nullptr); + auto* bi10 = fe.GetBlockInfo(10); + ASSERT_NE(bi10, nullptr); + EXPECT_EQ(bi10->true_head, 20u); + EXPECT_EQ(bi10->false_head, 30u); + EXPECT_EQ(bi10->premerge_head, 30u); } TEST_F(SpvParserTest, @@ -6932,26 +6939,11 @@ TEST_F(SpvParserTest, EXPECT_THAT(fe.block_order(), ElementsAre(10, 30, 20, 80, 99)); - auto* bi20 = fe.GetBlockInfo(20); - ASSERT_NE(bi20, nullptr); - ASSERT_NE(bi20->true_head_for, nullptr); - EXPECT_EQ(bi20->true_head_for->begin_id, 10u); - EXPECT_EQ(bi20->false_head_for, nullptr); - ASSERT_NE(bi20->premerge_head_for, nullptr); - EXPECT_EQ(bi20->premerge_head_for->begin_id, 10u); - - auto* bi30 = fe.GetBlockInfo(30); - ASSERT_NE(bi30, nullptr); - EXPECT_EQ(bi30->true_head_for, nullptr); - ASSERT_NE(bi30->false_head_for, nullptr); - EXPECT_EQ(bi30->false_head_for->begin_id, 10u); - EXPECT_EQ(bi30->premerge_head_for, nullptr); - - auto* bi80 = fe.GetBlockInfo(80); - ASSERT_NE(bi80, nullptr); - EXPECT_EQ(bi80->true_head_for, nullptr); - EXPECT_EQ(bi80->false_head_for, nullptr); - EXPECT_EQ(bi80->premerge_head_for, nullptr); + auto* bi10 = fe.GetBlockInfo(10); + ASSERT_NE(bi10, nullptr); + EXPECT_EQ(bi10->true_head, 20u); + EXPECT_EQ(bi10->false_head, 30u); + EXPECT_EQ(bi10->premerge_head, 20u); } TEST_F(SpvParserTest,