From 2871ad9138281edf4a37e0f27ef7d85ecfae967e Mon Sep 17 00:00:00 2001
From: David Neto <dneto@google.com>
Date: Tue, 1 Jun 2021 16:28:43 +0000
Subject: [PATCH] spirv-reader: emit null for value from unreachable block

Sometimes a value can come from a block which does not appear
in the structured block order.  That block will never execute,
so we can safely use the null value instead.

Bug: tint:804
Change-Id: Idc1a6c4f76f14a1d76919a95e5a65e56018ce68f
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/52840
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: James Price <jrprice@google.com>
Auto-Submit: David Neto <dneto@google.com>
---
 src/reader/spirv/function.cc           | 14 +++++++
 src/reader/spirv/function.h            |  5 ++-
 src/reader/spirv/function_misc_test.cc | 57 ++++++++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 7ded27d8aa..b55ecd7a76 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -1321,6 +1321,9 @@ void FunctionEmitter::ComputeBlockOrderAndPositions() {
   for (uint32_t i = 0; i < block_order_.size(); ++i) {
     GetBlockInfo(block_order_[i])->pos = i;
   }
+  // The invalid block position is not the position of any block that is in the
+  // order.
+  assert(block_order_.size() <= kInvalidBlockPos);
 }
 
 bool FunctionEmitter::VerifyHeaderContinueMergeOrder() {
@@ -2294,6 +2297,17 @@ TypedExpression FunctionEmitter::MakeExpression(uint32_t id) {
     default:
       break;
   }
+  if (const spvtools::opt::BasicBlock* const bb =
+          ir_context_.get_instr_block(id)) {
+    if (auto* block = GetBlockInfo(bb->id())) {
+      if (block->pos == kInvalidBlockPos) {
+        // The value came from a block not in the block order.
+        // Substitute a null value.
+        return parser_impl_.MakeNullExpression(
+            parser_impl_.ConvertType(inst->type_id()));
+      }
+    }
+  }
   Fail() << "unhandled expression for ID " << id << "\n" << inst->PrettyPrint();
   return {};
 }
diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h
index 4e316f452e..f0a2c975fd 100644
--- a/src/reader/spirv/function.h
+++ b/src/reader/spirv/function.h
@@ -70,6 +70,8 @@ enum class EdgeKind {
   kForward
 };
 
+enum : uint32_t { kInvalidBlockPos = ~(0u) };
+
 /// Bookkeeping info for a basic block.
 struct BlockInfo {
   /// Constructor
@@ -84,7 +86,8 @@ struct BlockInfo {
   uint32_t id = 0;
 
   /// The position of this block in the reverse structured post-order.
-  uint32_t pos = 0;
+  /// If the block is not in that order, then this remains the invalid value.
+  uint32_t pos = kInvalidBlockPos;
 
   /// If this block is a header, then this is the ID of the merge block.
   uint32_t merge_for_header = 0;
diff --git a/src/reader/spirv/function_misc_test.cc b/src/reader/spirv/function_misc_test.cc
index 5a9354845b..c22e4d1d0e 100644
--- a/src/reader/spirv/function_misc_test.cc
+++ b/src/reader/spirv/function_misc_test.cc
@@ -496,6 +496,63 @@ INSTANTIATE_TEST_SUITE_P(
         {4, "", "vector component index is larger than 3: 4"},
         {99999, "", "vector component index is larger than 3: 99999"}}));
 
+TEST_F(SpvParserTest, ValueFromBlockNotInBlockOrder) {
+  // crbug.com/tint/804
+  const auto assembly = Preamble() + CommonTypes() + R"(
+     %float_42 = OpConstant %float 42.0
+     %cond = OpUndef %bool
+
+     %100 = OpFunction %void None %voidfn
+     %10 = OpLabel
+     OpBranch %30
+
+     ; unreachable
+     %20 = OpLabel
+     %499 = OpFAdd %float %float_42 %float_42
+     %500 = OpFAdd %float %499 %float_42
+     OpBranch %25
+
+     %25 = OpLabel
+     OpBranch %80
+
+
+     %30 = OpLabel
+     OpLoopMerge %90 %80 None
+     OpBranchConditional %cond %90 %40
+
+     %40 = OpLabel
+     OpBranch %90
+
+     %80 = OpLabel ; unreachable continue target
+                ; but "dominated" by %20 and %25
+     %81 = OpFMul %float %500 %float_42 ; %500 is defined in %20
+     OpBranch %30 ; backedge
+
+     %90 = OpLabel
+     OpReturn
+     OpFunctionEnd
+)";
+  auto p = parser(test::Assemble(assembly));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+  auto fe = p->function_emitter(100);
+  EXPECT_TRUE(fe.EmitBody()) << p->error();
+  const auto got = ToString(p->builder(), fe.ast_body());
+  EXPECT_THAT(got, HasSubstr(R"(VariableDeclStatement{
+      VariableConst{
+        x_81
+        none
+        __f32
+        {
+          Binary[not set]{
+            ScalarConstructor[not set]{0.000000}
+            multiply
+            ScalarConstructor[not set]{42.000000}
+          }
+        }
+      }
+    })"));
+}
+
 // TODO(dneto): OpSizeof : requires Kernel (OpenCL)
 
 }  // namespace