Fix an SSA construction bug (#739)

Fixes #723

This fixes a case where the SSA construction pass wasn't dealing with the possibility of a phi node that it had provisionally introduced being replaced later. The result was invalid IR (caught with `-validate-ir`) that referenced an instruction nowhere in the IR module (because it was dropped).

The fix centralizes the code for dealing with phi nodes that have been replaced, so that the two different paths where variables get "read" during SSA construction can both use the same logic.

2 files changed, 65 insertions, 21 deletions

diff --git a/source/slang/ir-ssa.cpp b/source/slang/ir-ssa.cpp
index 207938a90..973030852 100644
--- a/source/slang/ir-ssa.cpp
+++ b/source/slang/ir-ssa.cpp
@@ -572,6 +572,43 @@ void maybeSealBlock(
     blockInfo->isSealed = true;
 }
 
+// In some cases we may have a pointer to an IR value that
+// represents a phi node that has been replaced with another
+// IR value, because we discovered that the phi is no longer
+// needed.
+//
+// The `maybeGetPhiReplacement` function will follow any
+// chain of replacements that might be present, so that we
+// don't end up referencing a dangling/unused value in
+// the code that we generate.
+//
+IRInst* maybeGetPhiReplacement(
+    ConstructSSAContext*    context,
+    IRInst*                 inVal)
+{
+    IRInst* val = inVal;
+
+    while( val->op == kIROp_Param )
+    {
+        // The value is a parameter, but is it a phi?
+        IRParam* maybePhi = (IRParam*) val;
+        RefPtr<PhiInfo> phiInfo = nullptr;
+        if(!context->phiInfos.TryGetValue(maybePhi, phiInfo))
+            break;
+
+        // Okay, this is indeed a phi we are adding, but
+        // is it one that got replaced?
+        if(!phiInfo->replacement)
+            break;
+
+        // The phi we want to use got replaced, so we
+        // had better use the replacement instead.
+        val = phiInfo->replacement;
+    }
+
+    return val;
+}
+
 IRInst* readVarRec(
     ConstructSSAContext*    context,
     SSABlockInfo*           blockInfo,
@@ -664,10 +701,24 @@ IRInst* readVarRec(
     // value for the given variable in this block
     writeVar(context, blockInfo, var, val);
 
+    // If `val` represents a phi node (block parameter) then
+    // it is possible that some of the operations above might
+    // have caused it to be replaced with another value,
+    // and in that case we had better not return it to
+    // be referenced in user code.
+    //
+    // Note: it is okay for the `valueForVar` map that
+    // we update in `writeVar` to use the old value, so long
+    // as we do this replacement logic anywhere we might read
+    // from that map.
+    //
+    val = maybeGetPhiReplacement(context, val);
+
     return val;
 }
 
 
+
 IRInst* readVar(
     ConstructSSAContext*    context,
     SSABlockInfo*           blockInfo,
@@ -682,27 +733,11 @@ IRInst* readVar(
         // Hooray, we found a value to use, and we
         // can proceed without too many complications.
 
-        // Well, let's check for one special case here, which
-        // is when the value we intend to use is a `phi`
-        // node that we ultimately decided to remove.
-        while( val->op == kIROp_Param )
-        {
-            // The value is a parameter, but is it a phi?
-            IRParam* maybePhi = (IRParam*) val;
-            RefPtr<PhiInfo> phiInfo = nullptr;
-            if(!context->phiInfos.TryGetValue(maybePhi, phiInfo))
-                break;
-
-            // Okay, this is indeed a phi we are adding, but
-            // is it one that got replaced?
-            if(!phiInfo->replacement)
-                break;
-
-            // The phi we want to use got replaced, so we
-            // had better use the replacement instead.
-            val = phiInfo->replacement;
-        }
-
+        // Just like in the `readVarRec` case above, we need
+        // to handle the case where `val` might represent
+        // a phi node that has subsequently been replaced.
+        //
+        val = maybeGetPhiReplacement(context, val);
 
         return val;
     }
diff --git a/source/slang/slang.natvis b/source/slang/slang.natvis
index bb3d3a16c..247064f3a 100644
--- a/source/slang/slang.natvis
+++ b/source/slang/slang.natvis
@@ -93,6 +93,15 @@
         </Expand>
       </Synthetic>
       <Item Name="[parent]">parent</Item>
+      <Synthetic Name="[children]" Condition="(op &gt;= Slang::kIROp_FirstParentInst) &amp;&amp; (op &lt;= Slang::kIROp_LastParentInst)">
+        <Expand>
+          <LinkedListItems>
+            <HeadPointer>(*((Slang::IRParentInst*) this)).children.first</HeadPointer>
+            <NextPointer>next</NextPointer>
+            <ValueNode>this</ValueNode>
+          </LinkedListItems>
+        </Expand>
+      </Synthetic>
       <Synthetic Name="[uses]">
         <Expand>
           <LinkedListItems>