[spirv-reader] Even better hoisted variables

Use the fact that in WGSL the scope corresponding to the loop construct
encloses the scope for its associated continue construct.
In our construct data structure, the two are adjacent but not
overlapping.

This improvement means that when a definition is in a loop construct,
but used only in the loop or associated continue construct, then no
hoisting is required.

Bug: tint:3
Change-Id: I8d33b8f76303ab2868306847e846b4c26899e746
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/24420
Reviewed-by: dan sinclair <dsinclair@google.com>

src/reader/spirv/construct.cc

@@ -24,7 +24,8 @@ Construct::Construct(const Construct* the_parent,
                      uint32_t the_begin_id,
                      uint32_t the_end_id,
                      uint32_t the_begin_pos,
-                     uint32_t the_end_pos)
+                     uint32_t the_end_pos,
+                     uint32_t the_scope_end_pos)
     : parent(the_parent),
       enclosing_loop(
           // Compute the enclosing loop construct. Doing this in the
@@ -61,7 +62,8 @@ Construct::Construct(const Construct* the_parent,
       begin_id(the_begin_id),
       end_id(the_end_id),
       begin_pos(the_begin_pos),
-      end_pos(the_end_pos) {}
+      end_pos(the_end_pos),
+      scope_end_pos(the_scope_end_pos) {}
 
 }  // namespace spirv
 }  // namespace reader

src/reader/spirv/construct.h

@@ -92,19 +92,31 @@ struct Construct {
   /// @param the_end_id block id of the first block after the construct, or 0
   /// @param the_begin_pos block order position of the_begin_id
   /// @param the_end_pos block order position of the_end_id or a too-large value
+  /// @param the_scope_end_pos block position of the first block past the end of
+  /// the WGSL scope
   Construct(const Construct* the_parent,
             int the_depth,
             Kind the_kind,
             uint32_t the_begin_id,
             uint32_t the_end_id,
             uint32_t the_begin_pos,
-            uint32_t the_end_pos);
+            uint32_t the_end_pos,
+            uint32_t the_scope_end_pos);
 
   /// @param pos a block position
   /// @returns true if the given block position is inside this construct.
   bool ContainsPos(uint32_t pos) const {
     return begin_pos <= pos && pos < end_pos;
   }
+  /// Returns true if the given block position is inside the WGSL scope
+  /// corresponding to this construct. A loop construct's WGSL scope encloses
+  /// the associated continue construct. Otherwise the WGSL scope extent is the
+  /// same as the block extent.
+  /// @param pos a block position
+  /// @returns true if the given block position is inside the WGSL scope.
+  bool ScopeContainsPos(uint32_t pos) const {
+    return begin_pos <= pos && pos < scope_end_pos;
+  }
 
   /// The nearest enclosing construct other than itself, or nullptr if
   /// this construct represents the entire function.
@@ -136,6 +148,9 @@ struct Construct {
   /// The position of block |end_id| in the block order, or the number of
   /// block order elements if |end_id| is 0.
   const uint32_t end_pos = 0;
+  /// The position of the first block after the WGSL scope corresponding to
+  /// this construct.
+  const uint32_t scope_end_pos = 0;
 };
 
 using ConstructList = std::vector<std::unique_ptr<Construct>>;
@@ -183,6 +198,10 @@ inline std::ostream& operator<<(std::ostream& o, const Construct& c) {
 
   o << " parent:" << ToStringBrief(c.parent);
 
+  if (c.scope_end_pos != c.end_pos) {
+    o << " scope:[" << c.begin_pos << "," << c.scope_end_pos << ")";
+  }
+
   if (c.enclosing_loop) {
     o << " in-l:" << ToStringBrief(c.enclosing_loop);
   }

src/reader/spirv/function.cc

@@ -903,16 +903,18 @@ bool FunctionEmitter::LabelControlFlowConstructs() {
     const auto end_pos =
         end_id == 0 ? uint32_t(block_order_.size()) : GetBlockInfo(end_id)->pos;
     const auto* parent = enclosing.empty() ? nullptr : enclosing.back();
+    auto scope_end_pos = end_pos;
     // A loop construct is added right after its associated continue construct.
     // In that case, adjust the parent up.
     if (k == Construct::kLoop) {
       assert(parent);
       assert(parent->kind == Construct::kContinue);
+      scope_end_pos = parent->end_pos;
       parent = parent->parent;
     }
-    constructs_.push_back(
+    constructs_.push_back(std::make_unique<Construct>(
-        std::make_unique<Construct>(parent, static_cast<int>(depth), k,
+        parent, static_cast<int>(depth), k, begin_id, end_id, begin_pos,
-                                    begin_id, end_id, begin_pos, end_pos));
+        end_pos, scope_end_pos));
     Construct* result = constructs_.back().get();
     enclosing.push_back(result);
     return result;
@@ -3137,7 +3139,8 @@ const Construct* FunctionEmitter::GetEnclosingScope(uint32_t first_pos,
       GetBlockInfo(block_order_[first_pos])->construct;
   assert(enclosing_construct != nullptr);
   // Constructs are strictly nesting, so follow parent pointers
-  while (enclosing_construct && !enclosing_construct->ContainsPos(last_pos)) {
+  while (enclosing_construct &&
+         !enclosing_construct->ScopeContainsPos(last_pos)) {
     // The scope of a continue construct is enclosed in its associated loop
     // construct, but they are siblings in our construct tree.
     const auto* sibling_loop = SiblingLoopConstruct(enclosing_construct);

src/reader/spirv/function_cfg_test.cc

@@ -3100,7 +3100,7 @@ TEST_F(SpvParserTest,
   EXPECT_THAT(ToString(constructs), Eq(R"(ConstructList{
   Construct{ Function [0,6) begin_id:10 end_id:0 depth:0 parent:null }
   Construct{ Continue [3,5) begin_id:40 end_id:99 depth:1 parent:Function@10 in-c:Continue@40 }
-  Construct{ Loop [1,3) begin_id:20 end_id:40 depth:1 parent:Function@10 in-l:Loop@20 }
+  Construct{ Loop [1,3) begin_id:20 end_id:40 depth:1 parent:Function@10 scope:[1,5) in-l:Loop@20 }
 })")) << constructs;
   // The block records the nearest enclosing construct.
   EXPECT_EQ(fe.GetBlockInfo(10)->construct, constructs[0].get());
@@ -3244,7 +3244,7 @@ TEST_F(SpvParserTest,
   Construct{ Function [0,5) begin_id:10 end_id:0 depth:0 parent:null }
   Construct{ IfSelection [0,2) begin_id:10 end_id:50 depth:1 parent:Function@10 }
   Construct{ Continue [3,4) begin_id:60 end_id:99 depth:1 parent:Function@10 in-c:Continue@60 }
-  Construct{ Loop [2,3) begin_id:50 end_id:60 depth:1 parent:Function@10 in-l:Loop@50 }
+  Construct{ Loop [2,3) begin_id:50 end_id:60 depth:1 parent:Function@10 scope:[2,4) in-l:Loop@50 }
 })")) << constructs;
   // The block records the nearest enclosing construct.
   EXPECT_EQ(fe.GetBlockInfo(10)->construct, constructs[1].get());
@@ -3466,7 +3466,7 @@ TEST_F(SpvParserTest, LabelControlFlowConstructs_Nest_Loop_Loop) {
   EXPECT_THAT(ToString(constructs), Eq(R"(ConstructList{
   Construct{ Function [0,8) begin_id:10 end_id:0 depth:0 parent:null }
   Construct{ Continue [4,6) begin_id:50 end_id:89 depth:1 parent:Function@10 in-c:Continue@50 }
-  Construct{ Loop [1,4) begin_id:20 end_id:50 depth:1 parent:Function@10 in-l:Loop@20 }
+  Construct{ Loop [1,4) begin_id:20 end_id:50 depth:1 parent:Function@10 scope:[1,6) in-l:Loop@20 }
   Construct{ Continue [2,3) begin_id:30 end_id:40 depth:2 parent:Loop@20 in-l:Loop@20 in-c:Continue@30 }
 })")) << constructs;
   // The block records the nearest enclosing construct.
@@ -3521,7 +3521,7 @@ TEST_F(SpvParserTest, LabelControlFlowConstructs_Nest_Loop_If) {
   EXPECT_THAT(ToString(constructs), Eq(R"(ConstructList{
   Construct{ Function [0,7) begin_id:10 end_id:0 depth:0 parent:null }
   Construct{ Continue [5,6) begin_id:80 end_id:99 depth:1 parent:Function@10 in-c:Continue@80 }
-  Construct{ Loop [1,5) begin_id:20 end_id:80 depth:1 parent:Function@10 in-l:Loop@20 }
+  Construct{ Loop [1,5) begin_id:20 end_id:80 depth:1 parent:Function@10 scope:[1,6) in-l:Loop@20 }
   Construct{ IfSelection [2,4) begin_id:30 end_id:49 depth:2 parent:Loop@20 in-l:Loop@20 }
 })")) << constructs;
   // The block records the nearest enclosing construct.
@@ -3572,7 +3572,7 @@ TEST_F(SpvParserTest, LabelControlFlowConstructs_Nest_LoopContinue_If) {
   EXPECT_THAT(ToString(constructs), Eq(R"(ConstructList{
   Construct{ Function [0,6) begin_id:10 end_id:0 depth:0 parent:null }
   Construct{ Continue [2,5) begin_id:30 end_id:99 depth:1 parent:Function@10 in-c:Continue@30 }
-  Construct{ Loop [1,2) begin_id:20 end_id:30 depth:1 parent:Function@10 in-l:Loop@20 }
+  Construct{ Loop [1,2) begin_id:20 end_id:30 depth:1 parent:Function@10 scope:[1,5) in-l:Loop@20 }
   Construct{ IfSelection [2,4) begin_id:30 end_id:49 depth:2 parent:Continue@30 in-c:Continue@30 }
 })")) << constructs;
   // The block records the nearest enclosing construct.
@@ -3666,7 +3666,7 @@ TEST_F(SpvParserTest, LabelControlFlowConstructs_Nest_If_MultiBlockLoop) {
   Construct{ Function [0,7) begin_id:10 end_id:0 depth:0 parent:null }
   Construct{ IfSelection [0,6) begin_id:10 end_id:99 depth:1 parent:Function@10 }
   Construct{ Continue [3,5) begin_id:40 end_id:89 depth:2 parent:IfSelection@10 in-c:Continue@40 }
-  Construct{ Loop [1,3) begin_id:20 end_id:40 depth:2 parent:IfSelection@10 in-l:Loop@20 }
+  Construct{ Loop [1,3) begin_id:20 end_id:40 depth:2 parent:IfSelection@10 scope:[1,5) in-l:Loop@20 }
 })")) << constructs;
   // The block records the nearest enclosing construct.
   EXPECT_EQ(fe.GetBlockInfo(10)->construct, constructs[1].get());
@@ -13538,7 +13538,7 @@ TEST_F(SpvParserTest, SiblingLoopConstruct_HasSiblingLoop) {
   EXPECT_EQ(c->kind, Construct::kContinue);
   EXPECT_THAT(ToString(fe.SiblingLoopConstruct(c)),
               Eq("Construct{ Loop [1,2) begin_id:20 end_id:30 depth:1 "
-                 "parent:Function@10 in-l:Loop@20 }"));
+                 "parent:Function@10 scope:[1,3) in-l:Loop@20 }"));
 }
 
 }  // namespace

src/reader/spirv/function_var_test.cc

@@ -1123,13 +1123,6 @@ TEST_F(SpvParserTest, EmitStatement_Phi_MultiBlockLoopIndex) {
   EXPECT_TRUE(fe.EmitBody()) << p->error();
 
   EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(Loop{
-  VariableDeclStatement{
-    Variable{
-      x_2
-      function
-      __u32
-    }
-  }
   VariableDeclStatement{
     Variable{
       x_2_phi
@@ -1181,9 +1174,15 @@ TEST_F(SpvParserTest, EmitStatement_Phi_MultiBlockLoopIndex) {
     }
   }
   Loop{
-    Assignment{
+    VariableDeclStatement{
-      Identifier{x_2}
+      Variable{
-      Identifier{x_2_phi}
+        x_2
+        none
+        __u32
+        {
+          Identifier{x_2_phi}
+        }
+      }
     }
     VariableDeclStatement{
       Variable{
@@ -1289,20 +1288,6 @@ TEST_F(SpvParserTest, EmitStatement_Phi_ValueFromLoopBodyAndContinuing) {
   }
 }
 Loop{
-  VariableDeclStatement{
-    Variable{
-      x_4
-      function
-      __u32
-    }
-  }
-  VariableDeclStatement{
-    Variable{
-      x_6
-      function
-      __u32
-    }
-  }
   VariableDeclStatement{
     Variable{
       x_2_phi
@@ -1346,20 +1331,32 @@ Loop{
         }
       }
     }
-    Assignment{
+    VariableDeclStatement{
-      Identifier{x_4}
+      Variable{
-      Binary{
+        x_4
-        Identifier{x_2}
+        none
-        add
+        __u32
-        ScalarConstructor{1}
+        {
+          Binary{
+            Identifier{x_2}
+            add
+            ScalarConstructor{1}
+          }
+        }
       }
     }
-    Assignment{
+    VariableDeclStatement{
-      Identifier{x_6}
+      Variable{
-      Binary{
+        x_6
-        Identifier{x_4}
+        none
-        add
+        __u32
-        ScalarConstructor{1}
+        {
+          Binary{
+            Identifier{x_4}
+            add
+            ScalarConstructor{1}
+          }
+        }
       }
     }
     If{