From 80767413b8fa35942a5e3b274fc94ad87aded823 Mon Sep 17 00:00:00 2001
From: David Neto <dneto@google.com>
Date: Thu, 8 Apr 2021 14:51:57 +0000
Subject: [PATCH] spirv-reader: fix position calculation of for phi var decl

Fixed: tint:495
Change-Id: I93f1b289eb33c2bda8b661f591a1c8dde775b1ab
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46220
Commit-Queue: David Neto <dneto@google.com>
Commit-Queue: Alan Baker <alanbaker@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: Alan Baker <alanbaker@google.com>
---
 src/reader/spirv/function.cc          |   2 +-
 src/reader/spirv/function_var_test.cc | 133 ++++++++++++++++++++++----
 2 files changed, 118 insertions(+), 17 deletions(-)

diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 5139f9fcdd..16012fe440 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -3961,7 +3961,7 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() {
             // of the predecessor block.
             pred_block_info->phi_assignments.push_back({phi_id, value_id});
             first_pos = std::min(first_pos, pred_block_info->pos);
-            last_pos = std::min(last_pos, pred_block_info->pos);
+            last_pos = std::max(last_pos, pred_block_info->pos);
           }
         }
 
diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc
index 790586c569..1543518430 100644
--- a/src/reader/spirv/function_var_test.cc
+++ b/src/reader/spirv/function_var_test.cc
@@ -1811,6 +1811,13 @@ VariableDeclStatement{
   }
 }
 Loop{
+  VariableDeclStatement{
+    Variable{
+      x_2_phi
+      function
+      __u32
+    }
+  }
   If{
     (
       Identifier[not set]{x_101}
@@ -1819,13 +1826,6 @@ Loop{
       Break{}
     }
   }
-  VariableDeclStatement{
-    Variable{
-      x_2_phi
-      function
-      __u32
-    }
-  }
   If{
     (
       Identifier[not set]{x_102}
@@ -1865,7 +1865,7 @@ Loop{
 }
 Return{}
 )";
-  EXPECT_EQ(expect, got);
+  EXPECT_EQ(expect, got) << got;
 }
 
 TEST_F(SpvParserTest, EmitStatement_Phi_FromHeaderAndThen) {
@@ -1933,6 +1933,13 @@ VariableDeclStatement{
   }
 }
 Loop{
+  VariableDeclStatement{
+    Variable{
+      x_2_phi
+      function
+      __u32
+    }
+  }
   If{
     (
       Identifier[not set]{x_101}
@@ -1941,13 +1948,6 @@ Loop{
       Break{}
     }
   }
-  VariableDeclStatement{
-    Variable{
-      x_2_phi
-      function
-      __u32
-    }
-  }
   Assignment{
     Identifier[not set]{x_2_phi}
     ScalarConstructor[not set]{0}
@@ -1982,7 +1982,108 @@ Loop{
 }
 Return{}
 )";
-  EXPECT_EQ(expect, got);
+  EXPECT_EQ(expect, got) << got;
+}
+
+TEST_F(SpvParserTest,
+       EmitStatement_Phi_InMerge_PredecessorsDominatdByNestedSwitchCase) {
+  // This is the essence of the bug report from crbug.com/tint/495
+  auto assembly = Preamble() + R"(
+     %cond = OpConstantTrue %bool
+     %pty = OpTypePointer Private %uint
+     %1 = OpVariable %pty Private
+     %boolpty = OpTypePointer Private %bool
+     %7 = OpVariable %boolpty Private
+     %8 = OpVariable %boolpty Private
+
+     %100 = OpFunction %void None %voidfn
+
+     %10 = OpLabel
+     OpSelectionMerge %99 None
+     OpSwitch %uint_1 %20 0 %20 1 %30
+
+       %20 = OpLabel ; case 0
+       OpBranch %30 ;; fall through
+
+       %30 = OpLabel ; case 1
+       OpSelectionMerge %50 None
+       OpBranchConditional %true %40 %45
+
+         %40 = OpLabel
+         OpBranch %50
+
+         %45 = OpLabel
+         OpBranch %99 ; break
+
+       %50 = OpLabel ; end the case
+       OpBranch %99
+
+     %99 = OpLabel
+     ; predecessors are all dominated by case construct head at %30
+     %phi = OpPhi %uint %uint_0 %45 %uint_1 %50
+     OpReturn
+
+     OpFunctionEnd
+  )";
+  auto p = parser(test::Assemble(assembly));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
+  FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
+  EXPECT_TRUE(fe.EmitBody()) << p->error();
+
+  auto got = ToString(p->builder(), fe.ast_body());
+  auto* expect = R"(VariableDeclStatement{
+  Variable{
+    x_35_phi
+    function
+    __u32
+  }
+}
+Switch{
+  ScalarConstructor[not set]{1}
+  {
+    Default{
+      Fallthrough{}
+    }
+    Case 0{
+      Fallthrough{}
+    }
+    Case 1{
+      If{
+        (
+          ScalarConstructor[not set]{true}
+        )
+        {
+        }
+      }
+      Else{
+        {
+          Assignment{
+            Identifier[not set]{x_35_phi}
+            ScalarConstructor[not set]{0}
+          }
+          Break{}
+        }
+      }
+      Assignment{
+        Identifier[not set]{x_35_phi}
+        ScalarConstructor[not set]{1}
+      }
+    }
+  }
+}
+VariableDeclStatement{
+  VariableConst{
+    x_35
+    none
+    __u32
+    {
+      Identifier[not set]{x_35_phi}
+    }
+  }
+}
+Return{}
+)";
+  EXPECT_EQ(expect, got) << got << assembly;
 }
 
 TEST_F(SpvParserTest, EmitStatement_UseInPhiCountsAsUse) {