From 742790c8ff7cc9a3608916d0869105542191a513 Mon Sep 17 00:00:00 2001 From: David Neto Date: Thu, 23 Apr 2020 20:28:34 +0000 Subject: [PATCH] Fix nits from review From review https://dawn-review.googlesource.com/c/tint/+/20080 - single_block_loop -> is_single_block_loop - better message for internal error for missing block when registering merges Bug: tint:3 Change-Id: Ief8ac5ce8ad7ffe93c28e0e7e2a793d50ce2de6c Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/20345 Reviewed-by: David Neto --- src/reader/spirv/function.cc | 15 +++++++------ src/reader/spirv/function.h | 4 ++-- src/reader/spirv/function_cfg_test.cc | 32 +++++++++++++-------------- 3 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 31fc1c7592..4883ebb494 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -390,7 +390,8 @@ bool FunctionEmitter::RegisterMerges() { const auto block_id = block.id(); auto* block_info = GetBlockInfo(block_id); if (!block_info) { - return Fail() << "internal error: assumed blocks were registered"; + return Fail() << "internal error: block " << block_id << " missing; blocks should already " + "have been registered"; } if (const auto* inst = block.GetMergeInst()) { @@ -464,18 +465,18 @@ bool FunctionEmitter::RegisterMerges() { } // Check single-block loop cases. - bool single_block_loop = false; + bool is_single_block_loop = false; block_info->basic_block->ForEachSuccessorLabel( - [&single_block_loop, block_id](const uint32_t succ) { + [&is_single_block_loop, block_id](const uint32_t succ) { if (block_id == succ) - single_block_loop = true; + is_single_block_loop = true; }); - block_info->single_block_loop = single_block_loop; + block_info->is_single_block_loop = is_single_block_loop; const auto ct = block_info->continue_for_header; - if (single_block_loop && ct != block_id) { + if (is_single_block_loop && ct != block_id) { return Fail() << "Block " << block_id << " branches to itself but is not its own continue target"; - } else if (!single_block_loop && ct == block_id) { + } else if (!is_single_block_loop && ct == block_id) { return Fail() << "Loop header block " << block_id << " declares itself as its own continue target, but " "does not branch to itself"; diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index 73d8cd837c..46f15d746c 100644 --- a/src/reader/spirv/function.h +++ b/src/reader/spirv/function.h @@ -64,7 +64,7 @@ struct BlockInfo { uint32_t header_for_continue = 0; /// Is this block a single-block loop: A loop header that declares itself /// as its own continue target, and has branch to itself. - bool single_block_loop = false; + bool is_single_block_loop = false; }; /// A FunctionEmitter emits a SPIR-V function onto a Tint AST module. @@ -113,7 +113,7 @@ class FunctionEmitter { /// @returns true if terminators are sane bool TerminatorsAreSane(); - /// Populates merge-header cross-links and the |single_block_loop| member + /// Populates merge-header cross-links and the |is_single_block_loop| member /// of BlockInfo. Also verifies that merge instructions go to blocks in /// the same function. Assumes basic blocks have been registered, and /// terminators are sane. diff --git a/src/reader/spirv/function_cfg_test.cc b/src/reader/spirv/function_cfg_test.cc index 397d1d3ee6..d86182d7c8 100644 --- a/src/reader/spirv/function_cfg_test.cc +++ b/src/reader/spirv/function_cfg_test.cc @@ -309,7 +309,7 @@ TEST_F(SpvParserTest, RegisterMerges_NoMerges) { EXPECT_EQ(bi->continue_for_header, 0u); EXPECT_EQ(bi->header_for_merge, 0u); EXPECT_EQ(bi->header_for_continue, 0u); - EXPECT_FALSE(bi->single_block_loop); + EXPECT_FALSE(bi->is_single_block_loop); } TEST_F(SpvParserTest, RegisterMerges_GoodSelectionMerge_BranchConditional) { @@ -340,7 +340,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodSelectionMerge_BranchConditional) { EXPECT_EQ(bi10->continue_for_header, 0u); EXPECT_EQ(bi10->header_for_merge, 0u); EXPECT_EQ(bi10->header_for_continue, 0u); - EXPECT_FALSE(bi10->single_block_loop); + EXPECT_FALSE(bi10->is_single_block_loop); // Middle block is neither header nor merge const auto* bi20 = fe.GetBlockInfo(20); @@ -349,7 +349,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodSelectionMerge_BranchConditional) { EXPECT_EQ(bi20->continue_for_header, 0u); EXPECT_EQ(bi20->header_for_merge, 0u); EXPECT_EQ(bi20->header_for_continue, 0u); - EXPECT_FALSE(bi20->single_block_loop); + EXPECT_FALSE(bi20->is_single_block_loop); // Merge block points to the header const auto* bi99 = fe.GetBlockInfo(99); @@ -358,7 +358,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodSelectionMerge_BranchConditional) { EXPECT_EQ(bi99->continue_for_header, 0u); EXPECT_EQ(bi99->header_for_merge, 10u); EXPECT_EQ(bi99->header_for_continue, 0u); - EXPECT_FALSE(bi99->single_block_loop); + EXPECT_FALSE(bi99->is_single_block_loop); } TEST_F(SpvParserTest, RegisterMerges_GoodSelectionMerge_Switch) { @@ -389,7 +389,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodSelectionMerge_Switch) { EXPECT_EQ(bi10->continue_for_header, 0u); EXPECT_EQ(bi10->header_for_merge, 0u); EXPECT_EQ(bi10->header_for_continue, 0u); - EXPECT_FALSE(bi10->single_block_loop); + EXPECT_FALSE(bi10->is_single_block_loop); // Middle block is neither header nor merge const auto* bi20 = fe.GetBlockInfo(20); @@ -398,7 +398,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodSelectionMerge_Switch) { EXPECT_EQ(bi20->continue_for_header, 0u); EXPECT_EQ(bi20->header_for_merge, 0u); EXPECT_EQ(bi20->header_for_continue, 0u); - EXPECT_FALSE(bi20->single_block_loop); + EXPECT_FALSE(bi20->is_single_block_loop); // Merge block points to the header const auto* bi99 = fe.GetBlockInfo(99); @@ -407,7 +407,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodSelectionMerge_Switch) { EXPECT_EQ(bi99->continue_for_header, 0u); EXPECT_EQ(bi99->header_for_merge, 10u); EXPECT_EQ(bi99->header_for_continue, 0u); - EXPECT_FALSE(bi99->single_block_loop); + EXPECT_FALSE(bi99->is_single_block_loop); } TEST_F(SpvParserTest, RegisterMerges_GoodLoopMerge_SingleBlockLoop) { @@ -438,7 +438,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodLoopMerge_SingleBlockLoop) { EXPECT_EQ(bi10->continue_for_header, 0u); EXPECT_EQ(bi10->header_for_merge, 0u); EXPECT_EQ(bi10->header_for_continue, 0u); - EXPECT_FALSE(bi10->single_block_loop); + EXPECT_FALSE(bi10->is_single_block_loop); // Single block loop is its own continue, and marked as single block loop. const auto* bi20 = fe.GetBlockInfo(20); @@ -447,7 +447,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodLoopMerge_SingleBlockLoop) { EXPECT_EQ(bi20->continue_for_header, 20u); EXPECT_EQ(bi20->header_for_merge, 0u); EXPECT_EQ(bi20->header_for_continue, 20u); - EXPECT_TRUE(bi20->single_block_loop); + EXPECT_TRUE(bi20->is_single_block_loop); // Merge block points to the header const auto* bi99 = fe.GetBlockInfo(99); @@ -456,7 +456,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodLoopMerge_SingleBlockLoop) { EXPECT_EQ(bi99->continue_for_header, 0u); EXPECT_EQ(bi99->header_for_merge, 20u); EXPECT_EQ(bi99->header_for_continue, 0u); - EXPECT_FALSE(bi99->single_block_loop); + EXPECT_FALSE(bi99->is_single_block_loop); } TEST_F(SpvParserTest, RegisterMerges_GoodLoopMerge_MultiBlockLoop_Branch) { @@ -493,7 +493,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodLoopMerge_MultiBlockLoop_Branch) { EXPECT_EQ(bi20->continue_for_header, 40u); EXPECT_EQ(bi20->header_for_merge, 0u); EXPECT_EQ(bi20->header_for_continue, 0u); - EXPECT_FALSE(bi20->single_block_loop); + EXPECT_FALSE(bi20->is_single_block_loop); // Continue block points to header const auto* bi40 = fe.GetBlockInfo(40); @@ -502,7 +502,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodLoopMerge_MultiBlockLoop_Branch) { EXPECT_EQ(bi40->continue_for_header, 0u); EXPECT_EQ(bi40->header_for_merge, 0u); EXPECT_EQ(bi40->header_for_continue, 20u); - EXPECT_FALSE(bi40->single_block_loop); + EXPECT_FALSE(bi40->is_single_block_loop); // Merge block points to the header const auto* bi99 = fe.GetBlockInfo(99); @@ -511,7 +511,7 @@ TEST_F(SpvParserTest, RegisterMerges_GoodLoopMerge_MultiBlockLoop_Branch) { EXPECT_EQ(bi99->continue_for_header, 0u); EXPECT_EQ(bi99->header_for_merge, 20u); EXPECT_EQ(bi99->header_for_continue, 0u); - EXPECT_FALSE(bi99->single_block_loop); + EXPECT_FALSE(bi99->is_single_block_loop); } TEST_F(SpvParserTest, @@ -549,7 +549,7 @@ TEST_F(SpvParserTest, EXPECT_EQ(bi20->continue_for_header, 40u); EXPECT_EQ(bi20->header_for_merge, 0u); EXPECT_EQ(bi20->header_for_continue, 0u); - EXPECT_FALSE(bi20->single_block_loop); + EXPECT_FALSE(bi20->is_single_block_loop); // Continue block points to header const auto* bi40 = fe.GetBlockInfo(40); @@ -558,7 +558,7 @@ TEST_F(SpvParserTest, EXPECT_EQ(bi40->continue_for_header, 0u); EXPECT_EQ(bi40->header_for_merge, 0u); EXPECT_EQ(bi40->header_for_continue, 20u); - EXPECT_FALSE(bi40->single_block_loop); + EXPECT_FALSE(bi40->is_single_block_loop); // Merge block points to the header const auto* bi99 = fe.GetBlockInfo(99); @@ -567,7 +567,7 @@ TEST_F(SpvParserTest, EXPECT_EQ(bi99->continue_for_header, 0u); EXPECT_EQ(bi99->header_for_merge, 20u); EXPECT_EQ(bi99->header_for_continue, 0u); - EXPECT_FALSE(bi99->single_block_loop); + EXPECT_FALSE(bi99->is_single_block_loop); } TEST_F(SpvParserTest, RegisterMerges_SelectionMerge_BadTerminator) {