Fix missing OpPhi after merge return. (#2248)

The function `UpdatePhiNodes` was being called inconsistently.  In one
case, the cfg had already been updated to include the new edge, and in
another place the cfg was not updated.  This caused the function to
miss flagging a block as needing new phi nodes.  I picked that the cfg
should not be updated before making the call.  I documented it, and
change the call sites to match.

Fixes #2207.

source/opt/merge_return_pass.cpp

@@ -360,7 +360,7 @@ bool MergeReturnPass::BreakFromConstruct(
     BasicBlock* block, BasicBlock* merge_block,
     std::unordered_set<BasicBlock*>* predicated,
     std::list<BasicBlock*>* order) {
-  // Make sure the cfg is build here.  If we don't then it becomes very hard
+  // Make sure the CFG is build here.  If we don't then it becomes very hard
   // to know which new blocks need to be updated.
   context()->BuildInvalidAnalyses(IRContext::kAnalysisCFG);
 
@@ -403,6 +403,7 @@ bool MergeReturnPass::BreakFromConstruct(
   // 1. Load of the return status flag
   // 2. Branch to |merge_block| (true) or old body (false)
   // 3. Update OpPhi instructions in |merge_block|.
+  // 4. Update the CFG.
   //
   // Sine we are branching to the merge block of the current construct, there is
   // no need for an OpSelectionMerge.
@@ -421,10 +422,6 @@ bool MergeReturnPass::BreakFromConstruct(
   builder.AddConditionalBranch(load_id, merge_block->id(), old_body->id(),
                                old_body->id());
 
-  // Update the cfg
-  cfg()->AddEdges(block);
-  cfg()->RegisterBlock(old_body);
-
   // 3. Update OpPhi instructions in |merge_block|.
   BasicBlock* merge_original_pred = MarkedSinglePred(merge_block);
   if (merge_original_pred == nullptr) {
@@ -433,6 +430,12 @@ bool MergeReturnPass::BreakFromConstruct(
     MarkForNewPhiNodes(merge_block, old_body);
   }
 
+  // 4. Update the CFG.  We do this after updating the OpPhi instructions
+  // because |UpdatePhiNodes| assumes the edge from |block| has not been added
+  // to the CFG yet.
+  cfg()->AddEdges(block);
+  cfg()->RegisterBlock(old_body);
+
   assert(old_body->begin() != old_body->end());
   assert(block->begin() != block->end());
   return true;

source/opt/merge_return_pass.h

@@ -275,6 +275,8 @@ class MergeReturnPass : public MemPass {
   // new edge from |new_source|.  The value for that edge will be an Undef. If
   // |target| only had a single predecessor, then it is marked as needing new
   // phi nodes.  See |MarkForNewPhiNodes|.
+  //
+  // The CFG must not include the edge from |new_source| to |target| yet.
   void UpdatePhiNodes(BasicBlock* new_source, BasicBlock* target);
 
   StructuredControlState& CurrentState() { return state_.back(); }

test/opt/pass_merge_return_test.cpp

@@ -1254,6 +1254,69 @@ TEST_F(MergeReturnPassTest, StructuredControlFlowPartialReplacePhi) {
   SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
   SinglePassRunAndMatch<MergeReturnPass>(before, false);
 }
+
+TEST_F(MergeReturnPassTest, GeneratePhiInOuterLoop) {
+  const std::string before =
+      R"(
+      ; CHECK: OpLoopMerge
+      ; CHECK: OpLoopMerge [[merge:%\w+]] [[continue:%\w+]]
+      ; CHECK: [[continue]] = OpLabel
+      ; CHECK-NEXT: [[undef:%\w+]] = OpUndef
+      ; CHECK: [[merge]] = OpLabel
+      ; CHECK-NEXT: [[phi:%\w+]] = OpPhi %bool [[undef]] [[continue]] {{%\w+}} {{%\w+}}
+      ; CHECK: OpCopyObject %bool [[phi]]
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %4 "main"
+               OpExecutionMode %4 OriginUpperLeft
+               OpSource ESSL 310
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+       %bool = OpTypeBool
+          %8 = OpTypeFunction %bool
+      %false = OpConstantFalse %bool
+          %4 = OpFunction %void None %3
+          %5 = OpLabel
+         %63 = OpFunctionCall %bool %9
+               OpReturn
+               OpFunctionEnd
+          %9 = OpFunction %bool None %8
+         %10 = OpLabel
+               OpBranch %31
+         %31 = OpLabel
+               OpLoopMerge %33 %34 None
+               OpBranch %32
+         %32 = OpLabel
+               OpSelectionMerge %34 None
+               OpBranchConditional %false %46 %34
+         %46 = OpLabel
+               OpLoopMerge %51 %52 None
+               OpBranch %53
+         %53 = OpLabel
+               OpBranchConditional %false %50 %51
+         %50 = OpLabel
+               OpReturnValue %false
+         %52 = OpLabel
+               OpBranch %46
+         %51 = OpLabel
+               OpBranch %34
+         %34 = OpLabel
+         %64 = OpUndef %bool
+               OpBranchConditional %false %31 %33
+         %33 = OpLabel
+               OpBranch %28
+         %28 = OpLabel
+         %60 = OpCopyObject %bool %64
+               OpBranch %17
+         %17 = OpLabel
+               OpReturnValue %false
+               OpFunctionEnd
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  SinglePassRunAndMatch<MergeReturnPass>(before, false);
+}
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools