elim-multi-store: only patch loop header phis that we created

There can already be OpPhi instructions in a loop header that
are unrelated to the optimization.  We should not be patching those.

Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/826

source/opt/local_ssa_elim_pass.cpp

@@ -280,6 +280,8 @@ void LocalMultiStoreElimPass::SSABlockInitLoopHeader(
     const uint32_t phiId = TakeNextId();
     std::unique_ptr<ir::Instruction> newPhi(
       new ir::Instruction(SpvOpPhi, typeId, phiId, phi_in_operands));
+    // The only phis requiring patching are the ones we create.
+    phis_to_patch_.insert(phiId);
     // Only analyze the phi define now; analyze the phi uses after the
     // phi backedge predecessor value is patched.
     def_use_mgr_->AnalyzeInstDef(&*newPhi);
@@ -379,7 +381,11 @@ void LocalMultiStoreElimPass::PatchPhis(uint32_t header_id, uint32_t back_id) {
   ir::BasicBlock* header = id2block_[header_id];
   auto phiItr = header->begin();
   for (; phiItr->opcode() == SpvOpPhi; ++phiItr) {
-    // find phi operand index for back edge
+    // Only patch phis that we created in a loop header.
+    // There might be other phis unrelated to our optimizations.
+    if (0 == phis_to_patch_.count(phiItr->result_id())) continue;
+
+    // Find phi operand index for back edge
     uint32_t cnt = 0;
     uint32_t idx = phiItr->NumInOperands();
     phiItr->ForEachInId([&cnt,&back_id,&idx](uint32_t* iid) {
@@ -391,10 +397,11 @@ void LocalMultiStoreElimPass::PatchPhis(uint32_t header_id, uint32_t back_id) {
     // map. Use undef if variable not in map.
     const uint32_t varId = phiItr->GetSingleWordInOperand(idx);
     const auto valItr = label2ssa_map_[back_id].find(varId);
-    uint32_t valId = (valItr != label2ssa_map_[back_id].end()) ?
-      valItr->second :
-      Type2Undef(GetPointeeTypeId(def_use_mgr_->GetDef(varId)));
-    phiItr->SetInOperand(idx, { valId });
+    uint32_t valId =
+        (valItr != label2ssa_map_[back_id].end())
+            ? valItr->second
+            : Type2Undef(GetPointeeTypeId(def_use_mgr_->GetDef(varId)));
+    phiItr->SetInOperand(idx, {valId});
     // Analyze uses now that they are complete
     def_use_mgr_->AnalyzeInstUse(&*phiItr);
   }
@@ -509,6 +516,7 @@ void LocalMultiStoreElimPass::Initialize(ir::Module* module) {
   block2structured_succs_.clear();
   label2preds_.clear();
   label2ssa_map_.clear();
+  phis_to_patch_.clear();
 
   // Start new ids with next availablein module
   next_id_ = module_->id_bound();

source/opt/local_ssa_elim_pass.h

@@ -176,6 +176,10 @@ class LocalMultiStoreElimPass : public MemPass {
   std::unordered_map<uint32_t, std::unordered_map<uint32_t, uint32_t>>
       label2ssa_map_;
 
+  // The Ids of OpPhi instructions that are in a loop header and which require
+  // patching of the value for the loop back-edge.
+  std::unordered_set<uint32_t> phis_to_patch_;
+
   // Extra block whose successors are all blocks with no predecessors
   // in function.
   ir::BasicBlock pseudo_entry_block_;

test/opt/local_ssa_elim_test.cpp

@@ -1227,6 +1227,37 @@ OpFunctionEnd
       true, true);
 }
 
+TEST_F(LocalSSAElimTest, DontPatchPhiInLoopHeaderThatIsNotAVar) {
+  // From https://github.com/KhronosGroup/SPIRV-Tools/issues/826
+  // Don't try patching the (%16 %7) value/predecessor pair in the OpPhi.
+  // That OpPhi is unrelated to this optimization: we did not set that up
+  // in the SSA initialization for the loop header block.
+  // The pass should be a no-op on this module.
+
+  const std::string before = R"(OpCapability Shader
+OpMemoryModel Logical GLSL450
+OpEntryPoint GLCompute %1 "main"
+%void = OpTypeVoid
+%3 = OpTypeFunction %void
+%float = OpTypeFloat 32
+%float_1 = OpConstant %float 1
+%1 = OpFunction %void None %3
+%6 = OpLabel
+OpBranch %7
+%7 = OpLabel
+%8 = OpPhi %float %float_1 %6 %9 %7
+%9 = OpFAdd %float %8 %float_1
+OpLoopMerge %10 %7 None
+OpBranch %7
+%10 = OpLabel
+OpReturn
+OpFunctionEnd
+)";
+
+  SinglePassRunAndCheck<opt::LocalMultiStoreElimPass>(before, before, true,
+                                                      true);
+}
+
 // TODO(greg-lunarg): Add tests to verify handling of these cases:
 //
 //    No optimization in the presence of