Fix a bug in SSA form creation (#1058)

It was possible for a recursive call to `tryRemoveTriviailPhi` to remove/replace a phi node that was already in a list of removal candidates to be processed. When the recursive call returned and that candidate was again considered, its operands would have already been cleared, leading to an assertion failure.

This case is what was coming up in practice in a user shader, although I have not been able to reproduce the failure with a more minimal synthetic test yet.

This change also changes the SSA creation logic to avoid a runtime crash in the case of a trivial phi that only references itself (which was how the above bug surfaced to the user). The "fix" there is not ideal (it leaves a trivial phi behind), but should be enough to retain semantic correctness if user code ever causes that (corneer-case) code path to execute. It is also expected that such a trivial phi would be removed in later DCE passes anyway.

1 files changed, 19 insertions, 1 deletions

diff --git a/source/slang/slang-ir-ssa.cpp b/source/slang/slang-ir-ssa.cpp
index 5f42702ce..08a5ffa67 100644
--- a/source/slang/slang-ir-ssa.cpp
+++ b/source/slang/slang-ir-ssa.cpp
@@ -456,7 +456,14 @@ IRInst* tryRemoveTrivialPhi(
         // There were no operands other than the phi itself.
         // This implies that the value at the use sites should
         // actually be undefined.
-        SLANG_UNIMPLEMENTED_X("trivial phi");
+        //
+        // For now we will simply return in this case, without
+        // removing the phi node.
+        //
+        // TODO: Construct a value for `same` that represents an
+        // undefined value with the same type as `phi`.
+        //
+        return phi;
     }
 
     // Removing this phi as trivial may make other phi nodes
@@ -498,6 +505,16 @@ IRInst* tryRemoveTrivialPhi(
     // other phis that might have become  trivial.
     for( auto otherPhi : otherPhis )
     {
+        // It is possible that between when we added a phi
+        // node to `otherPhis` and here it might have been
+        // replaced. This could happen if `otherPhis` had
+        // two entries, `A` and `B`, and the recursive
+        // `tryRemoveTrivialPhi` on `A` also ended up
+        // eliminating `B`.
+        //
+        if( otherPhi->replacement )
+            continue;
+
         tryRemoveTrivialPhi(context, otherPhi);
     }
 
@@ -524,6 +541,7 @@ IRInst* addPhiOperands(
         auto predInfo = *context->blockInfos.TryGetValue(predBlock);
 
         auto phiOperand = readVar(context, predInfo, var);
+        SLANG_ASSERT(phiOperand != nullptr);
 
         operandValues.add(phiOperand);
     }