Falcor fixes (#402)

* Re-define deprecated compile flags

By including these flags in the header file, with a value of zero, we can allow some existing code to compile even after the major changes to the implementation.

* The `SLANG_COMPILE_FLAG_NO_CHECKING` option will effectively be ignored, since checking is always enabled.

* The `SLANG_COMPILE_FLAG_SPLIT_MIXED_TYPES` option will now act as if it is always enabled (and indeed some of the code has been relying on this flag being set always).

* Make subscript operators writable for writable textures

This even had a `TODO` comment saying that we needed to fix it, and now I'm seeing semantic checking failures because we didn't define these and so we find assignment to non l-values.

* Fix definitions of any() and all() intrinsics

These should always return a scalar `bool` value, but they were being defined wrong in two ways:

1. They were using their generic type parameter `T` in the return type
2. They were returning a vector in the vector case, and a matrix in the matrix case.

This change just alters the return type to be `bool` in all cases.

* Fix bug in SSA construction

When eliminating a trivial phi node, it is possible that the phi is still recorded as the "latest" value for a local variable in its block.
When later code queries that value from the block (which can happen whenever another block looks up a variable in its predecessors), it would get the old phi and not the replacement value.

I simply added a loop that checks if the value we look up is a phi that got replaced, and then continues with the replacement value (which might itself be a phi...). A more advanced solution might try to get clever and have the map itself hold `IRUse` values so that we can replace them seamlessly.

* Simplify IR control flow representation

This change gets rid of various special-case operations for conditional and unconditional branches, and instead requires emit logic to recognize when a direct branch is targetting a `break` or `continue` label.

The new approach here isn't perfect, but it seems beter than what we had before, because it can actually work in the presence of control-flow optimizations (including our current critical-edge-splitting step).

* Load from groupshared isn't groupshared

When loading from a `groupshared` variable, the resulting temporary shouldn't have the `groupshared` qualifier on it.
This might eventually need to generalize to a better understanding of storage modifiers in the IR, but I don't really want to deal with that right now.

* Don't emit references to typedefs in output code

Now that we are using the IR for all codegen, we shouldn't be dealing with surface-level things like `typedef` declarations in the output code; just use the type that was being referred to in the first place.

* Fix floating-point literal printing for IR

The IR was calling `emit()` instead of `Emit()` (we really need to normalize our convention here), and was implicitly invoking a default constructor on `String` that takes a `double` (that constructor should really be marked `explicit`), and which doesn't meet our requirements for printing floating-point values.

* Fix error when importing module that doesn't parse

We already added a case to bail out if semantic checking fails, but neglected to add a case if there is an error during parsing of a module to be imported.

Note: this logic doesn't correctly register the module as being loaded (but still in error), so users could see multiple error messages if there are multiple `import`s for the same module.

* Improve error message for overload resolution failure

- Drop debugging info from the candidate printing

- Add cases to print `double` and `half` types properly

* Fixup: switch loopTest to ifElse in expected IR output

12 files changed, 270 insertions, 197 deletions

diff --git a/source/slang/check.cpp b/source/slang/check.cpp
index d26c5693f..40461d0a3 100644
--- a/source/slang/check.cpp
+++ b/source/slang/check.cpp
@@ -5961,6 +5961,8 @@ namespace Slang
 
 //                        declString = declString + "[" + String(candidate.conversionCostSum) + "]";
 
+#if 0
+                        // Debugging: ensure that we don't consider multiple declarations of the same operation
                         if (auto decl = dynamic_cast<CallableDecl*>(candidate.item.declRef.decl))
                         {
                             char buffer[1024];
@@ -5970,6 +5972,7 @@ namespace Slang
                                 decl->nextDecl);
                             declString.append(buffer);
                         }
+#endif
 
                         getSink()->diagnose(candidate.item.declRef, Diagnostics::overloadCandidate, declString);
 
diff --git a/source/slang/core.meta.slang b/source/slang/core.meta.slang
index 13b41bce6..f531d0f93 100644
--- a/source/slang/core.meta.slang
+++ b/source/slang/core.meta.slang
@@ -627,16 +627,28 @@ for (int tt = 0; tt < kBaseTextureTypeCount; ++tt)
 
             if(baseShape != TextureType::ShapeCube)
             {
-				// TODO: In the case where `access` includes writeability,
-				// this should have both `get` and `set` accessors.
-
                 // subscript operator
                 sb << "__subscript(uint";
 				if(kBaseTextureTypes[tt].coordCount + isArray > 1)
 				{
 					sb << kBaseTextureTypes[tt].coordCount + isArray;
 				}
-				sb << " location) -> T;\n";
+				sb << " location) -> T";
+
+                // Depending on the access level of the texture type,
+                // we either have just a getter (the default), or both
+                // a getter and setter.
+                switch( access )
+                {
+                case SLANG_RESOURCE_ACCESS_NONE:
+                case SLANG_RESOURCE_ACCESS_READ:
+                    sb << ";\n";
+                    break;
+
+                default:
+                    sb << " { get; set; }\n";
+                    break;
+                }
             }
 
             if( !isMultisample )
diff --git a/source/slang/core.meta.slang.h b/source/slang/core.meta.slang.h
index 84a1abbf0..03d796301 100644
--- a/source/slang/core.meta.slang.h
+++ b/source/slang/core.meta.slang.h
@@ -630,16 +630,28 @@ for (int tt = 0; tt < kBaseTextureTypeCount; ++tt)
 
             if(baseShape != TextureType::ShapeCube)
             {
-				// TODO: In the case where `access` includes writeability,
-				// this should have both `get` and `set` accessors.
-
                 // subscript operator
                 sb << "__subscript(uint";
 				if(kBaseTextureTypes[tt].coordCount + isArray > 1)
 				{
 					sb << kBaseTextureTypes[tt].coordCount + isArray;
 				}
-				sb << " location) -> T;\n";
+				sb << " location) -> T";
+
+                // Depending on the access level of the texture type,
+                // we either have just a getter (the default), or both
+                // a getter and setter.
+                switch( access )
+                {
+                case SLANG_RESOURCE_ACCESS_NONE:
+                case SLANG_RESOURCE_ACCESS_READ:
+                    sb << ";\n";
+                    break;
+
+                default:
+                    sb << " { get; set; }\n";
+                    break;
+                }
             }
 
             if( !isMultisample )
diff --git a/source/slang/emit.cpp b/source/slang/emit.cpp
index eaabd520c..0a71c7fa9 100644
--- a/source/slang/emit.cpp
+++ b/source/slang/emit.cpp
@@ -109,8 +109,6 @@ struct SharedEmitContext
 
     HashSet<String> irDeclsVisited;
 
-    Dictionary<IRBlock*, IRBlock*> irMapContinueTargetToLoopHead;
-
     HashSet<String> irTupleTypes;
 };
 
@@ -1095,17 +1093,14 @@ struct EmitVisitor
     UNEXPECTED(PtrType);
 
 #undef UNEXPECTED
+
     void visitNamedExpressionType(NamedExpressionType* type, TypeEmitArg const& arg)
     {
-        // Named types are valid for GLSL
-        if (context->shared->target == CodeGenTarget::GLSL)
-        {
-            emitTypeImpl(GetType(type->declRef), arg.declarator);
-            return;
-        }
+        // We will always emit the actual type referenced by
+        // a named type declaration, rather than try to produce
+        // equivalent `typedef` declarations in the output.
 
-        EmitDeclRef(type->declRef);
-        EmitDeclarator(arg.declarator);
+        emitTypeImpl(GetType(type->declRef), arg.declarator);
     }
 
     void visitBasicExpressionType(BasicExpressionType* basicType, TypeEmitArg const& arg)
@@ -4808,7 +4803,7 @@ emitDeclImpl(decl, nullptr);
             break;
 
         case kIROp_FloatLit:
-            emit(((IRConstant*) inst)->u.floatVal);
+            Emit(((IRConstant*) inst)->u.floatVal);
             break;
 
         case kIROp_boolConst:
@@ -6241,6 +6236,21 @@ emitDeclImpl(decl, nullptr);
         }
     }
 
+    enum LabelOp
+    {
+        kLabelOp_break,
+        kLabelOp_continue,
+
+        kLabelOpCount,
+    };
+
+    struct LabelStack
+    {
+        LabelStack* parent;
+        IRBlock*    block;
+        LabelOp     op;
+    };
+
     // We want to emit a range of code in the IR, represented
     // by the blocks that are logically in the interval [begin, end)
     // which we consider as a single-entry multiple-exit region.
@@ -6253,11 +6263,69 @@ emitDeclImpl(decl, nullptr);
     void emitIRStmtsForBlocks(
         EmitContext*    ctx,
         IRBlock*        begin,
-        IRBlock*        end)
+        IRBlock*        end,
+
+        // Labels to use at the start
+        LabelStack*     initialLabels,
+
+        // Labels to switch to after emitting first basic block
+        LabelStack*     labels = nullptr)
     {
+        if(!labels)
+            labels = initialLabels;
+
+        auto useLabels = initialLabels;
+
         IRBlock* block = begin;
         while(block != end)
         {
+            // If the block we are trying to emit has been registered as a
+            // destination label (e.g. for a loop or `switch`) then we
+            // may need to emit a `break` or `continue` as needed.
+            //
+            // TODO: we eventually need to handle the possibility of
+            // multi-level break/continue targets, which could be challenging.
+
+            // First, figure out which block has been registered as
+            // the current `break` and `continue` target.
+            IRBlock* registeredBlock[kLabelOpCount] = {};
+            for( auto ll = labels; ll; ll = ll->parent )
+            {
+                if(!registeredBlock[ll->op])
+                {
+                    registeredBlock[ll->op] = ll->block;
+                }
+            }
+
+            // Next, search in the active labels we are allowed to use,
+            // and see if the block we are trying to branch to is an
+            // available break/continue target.
+            for(auto ll = useLabels; ll; ll = ll->parent)
+            {
+                if(ll->block == block)
+                {
+                    // We are trying to go to a block that has been regsitered as a label.
+
+                    if(block != registeredBlock[ll->op])
+                    {
+                        // ERROR: need support for multi-level break/continue to pull this one off!
+                    }
+
+                    switch(ll->op)
+                    {
+                    case kLabelOp_break:
+                        emit("break;\n");
+                        break;
+
+                    case kLabelOp_continue:
+                        emit("continue;\n");
+                        break;
+                    }
+
+                    return;
+                }
+            }
+
             // Start by emitting the non-terminator instructions in the block.
             auto terminator = block->getLastInst();
             assert(isTerminatorInst(terminator));
@@ -6285,28 +6353,6 @@ emitDeclImpl(decl, nullptr);
                 emitIRInst(ctx, terminator);
                 return;
 
-            case kIROp_if:
-                {
-                    // One-sided `if` statement
-                    auto t = (IRIf*)terminator;
-
-                    auto trueBlock = t->getTrueBlock();
-                    auto afterBlock = t->getAfterBlock();
-
-                    emit("if(");
-                    emitIROperand(ctx, t->getCondition());
-                    emit(")\n{\n");
-                    emitIRStmtsForBlocks(
-                        ctx,
-                        trueBlock,
-                        afterBlock);
-                    emit("}\n");
-
-                    // Continue with the block after the `if`
-                    block = afterBlock;
-                }
-                break;
-
             case kIROp_ifElse:
                 {
                     // Two-sided `if` statement
@@ -6316,19 +6362,31 @@ emitDeclImpl(decl, nullptr);
                     auto falseBlock = t->getFalseBlock();
                     auto afterBlock = t->getAfterBlock();
 
+                    // TODO: consider simplifying the code in
+                    // the case where `trueBlock == afterBlock`
+                    // so that we output `if(!cond) { falseBlock }`
+                    // instead of the current `if(cond) {} else {falseBlock}`
+
                     emit("if(");
                     emitIROperand(ctx, t->getCondition());
                     emit(")\n{\n");
                     emitIRStmtsForBlocks(
                         ctx,
                         trueBlock,
-                        afterBlock);
-                    emit("}\nelse\n{\n");
-                    emitIRStmtsForBlocks(
-                        ctx,
-                        falseBlock,
-                        afterBlock);
+                        afterBlock,
+                        labels);
                     emit("}\n");
+                    // Don't emit the false block if it would be empty
+                    if(falseBlock != afterBlock)
+                    {
+                        emit("else\n{\n");
+                        emitIRStmtsForBlocks(
+                            ctx,
+                            falseBlock,
+                            afterBlock,
+                            labels);
+                        emit("}\n");
+                    }
 
                     // Continue with the block after the `if`
                     block = afterBlock;
@@ -6342,7 +6400,6 @@ emitDeclImpl(decl, nullptr);
 
                     auto targetBlock = t->getTargetBlock();
                     auto breakBlock = t->getBreakBlock();
-                    auto continueBlock = t->getContinueBlock();
 
                     UInt argCount = t->getArgCount();
                     static const UInt kFixedArgCount = 3;
@@ -6352,6 +6409,25 @@ emitDeclImpl(decl, nullptr);
                         t->getArgs() + kFixedArgCount,
                         targetBlock);
 
+                    // Set up entries on our label stack for break/continue
+
+                    LabelStack subBreakLabel;
+                    subBreakLabel.parent = labels;
+                    subBreakLabel.block = breakBlock;
+                    subBreakLabel.op = kLabelOp_break;
+
+                    // Note: when forming the `continue` label, we don't
+                    // actually point at the "continue block" from the loop
+                    // statement, because we aren't actually going to
+                    // generate an ordinary continue caluse in a `for` loop.
+                    //
+                    // Instead, our `continue` label will always be the
+                    // loop header.
+                    LabelStack subContinueLabel;
+                    subContinueLabel.parent = &subBreakLabel;
+                    subContinueLabel.block = targetBlock;
+                    subContinueLabel.op = kLabelOp_continue;
+
                     if (auto loopControlDecoration = t->findDecoration<IRLoopControlDecoration>())
                     {
                         switch (loopControlDecoration->mode)
@@ -6365,75 +6441,25 @@ emitDeclImpl(decl, nullptr);
                         }
                     }
 
-                    // The challenging case for a loop is when
-                    // there is a `continue` block that we
-                    // need to deal with.
-                    //
-                    if (continueBlock == targetBlock)
-                    {
-                        // There is no continue block, so
-                        // we only need to emit an endless
-                        // loop and then manually `break`
-                        // out of it in the right place(s)
-                        emit("for(;;)\n{\n");
+                    emit("for(;;)\n{\n");
 
-                        emitIRStmtsForBlocks(
-                            ctx,
-                            targetBlock,
-                            nullptr);
-
-                        emit("}\n");
-                    }
-                    else
-                    {
-                        // Okay, we've got a `continue` block,
-                        // which means we really want to emit
-                        // something akin to:
-                        //
-                        //     for(;; <continueBlock>) { <bodyBlock> }
-                        //
-                        // In principle this isn't so bad, since the
-                        // first case is just interVal [`continueBlock`, `targetBlock`)
-                        // and the latter is the interval [`targetBlock`, `continueBlock`).
-                        //
-                        // The challenge of course is that a `for` statement
-                        // only supports *expressions* in the continue part,
-                        // and we might have expanded things into multiple
-                        // instructions (especially if we inlined or desugared anything).
-                        //
-                        // There are a variety of ways we can support lowering this,
-                        // but for now we are going to do something expedient
-                        // that mimics what `fxc` seems to do:
-                        //
-                        // - Output loop body as `for(;;) { <bodyBlock> <continueBlock> }`
-                        // - At any `continue` site, output `{ <continueBlock>; continue; }`
-                        //
-                        // This isn't ideal because it leads to code duplication, but
-                        // it matches what `fxc` does so hopefully it will be the
-                        // best option for our tests.
-                        //
-
-                        emit("for(;;)\n{\n");
-
-                        // Register information so that `continue` sites
-                        // can do the right thing:
-                        ctx->shared->irMapContinueTargetToLoopHead.Add(continueBlock, targetBlock);
-
-
-                        emitIRStmtsForBlocks(
-                            ctx,
-                            targetBlock,
-                            nullptr);
-
-                        emit("}\n");
+                    emitIRStmtsForBlocks(
+                        ctx,
+                        targetBlock,
+                        nullptr,
+                        // For the first block, we only want the `break` label active
+                        &subBreakLabel,
+                        // After the first block, we can safely use the `continue` label too
+                        &subContinueLabel);
 
-                    }
+                    emit("}\n");
 
                     // Continue with the block after the loop
                     block = breakBlock;
                 }
                 break;
 
+#if 0
             case kIROp_break:
                 {
                     auto t = (IRBreak*)terminator;
@@ -6506,6 +6532,7 @@ emitDeclImpl(decl, nullptr);
                     block = afterBlock;
                 }
                 break;
+#endif
 
             case kIROp_unconditionalBranch:
                 {
@@ -6561,6 +6588,12 @@ emitDeclImpl(decl, nullptr);
                     auto breakLabel = t->getBreakLabel();
                     auto defaultLabel = t->getDefaultLabel();
 
+                    // Register the block to be used for our `break` target
+                    LabelStack subLabels;
+                    subLabels.parent = labels;
+                    subLabels.op = kLabelOp_break;
+                    subLabels.block = breakLabel;
+
                     // We need to track whether we've dealt with
                     // the `default` case already.
                     bool defaultLabelHandled = false;
@@ -6645,7 +6678,7 @@ emitDeclImpl(decl, nullptr);
 
                         // Now emit the statements for this case.
                         emit("{\n");
-                        emitIRStmtsForBlocks(ctx, caseLabel, caseEndLabel);
+                        emitIRStmtsForBlocks(ctx, caseLabel, caseEndLabel, &subLabels);
                         emit("}\n");
                     }
 
@@ -6656,7 +6689,7 @@ emitDeclImpl(decl, nullptr);
                     {
                         emit("default:\n");
                         emit("{\n");
-                        emitIRStmtsForBlocks(ctx, defaultLabel, breakLabel);
+                        emitIRStmtsForBlocks(ctx, defaultLabel, breakLabel, &subLabels);
                         emit("break;\n");
                         emit("}\n");
                     }
@@ -6668,6 +6701,11 @@ emitDeclImpl(decl, nullptr);
                 break;
             }
 
+            // After we've emitted the first block, we are safe from accidental
+            // cases where we'd emit an entire loop body as a single `continue`,
+            // so we can safely switch in whatever labels are intended to be used.
+            useLabels = labels;
+
             // If we reach this point, then we've emitted
             // one block, and we have a new block where
             // control flow continues.
@@ -6928,7 +6966,7 @@ emitDeclImpl(decl, nullptr);
 
             // Need to emit the operations in the blocks of the function
 
-            emitIRStmtsForBlocks(ctx, func->getFirstBlock(), nullptr);
+            emitIRStmtsForBlocks(ctx, func->getFirstBlock(), nullptr, nullptr);
 
             emit("}\n");
         }
@@ -7612,7 +7650,7 @@ emitDeclImpl(decl, nullptr);
             initFuncName.append("_init");
             emitIRType(ctx, varType, initFuncName);
             Emit("()\n{\n");
-            emitIRStmtsForBlocks(ctx, varDecl->firstBlock, nullptr);
+            emitIRStmtsForBlocks(ctx, varDecl->firstBlock, nullptr, nullptr);
             Emit("}\n");
         }
 
diff --git a/source/slang/hlsl.meta.slang b/source/slang/hlsl.meta.slang
index 7c5f26ed7..0f1263332 100644
--- a/source/slang/hlsl.meta.slang
+++ b/source/slang/hlsl.meta.slang
@@ -239,9 +239,9 @@ __generic<T : __BuiltinFloatingPointType, let N : int> vector<T,N> acos(vector<T
 __generic<T : __BuiltinFloatingPointType, let N : int, let M : int> matrix<T,N,M> acos(matrix<T,N,M> x);
 
 // Test if all components are non-zero (HLSL SM 1.0)
-__generic<T : __BuiltinType> T all(T x);
-__generic<T : __BuiltinType, let N : int> vector<T,N> all(vector<T,N> x);
-__generic<T : __BuiltinType, let N : int, let M : int> matrix<T,N,M> all(matrix<T,N,M> x);
+__generic<T : __BuiltinType> bool all(T x);
+__generic<T : __BuiltinType, let N : int> bool all(vector<T,N> x);
+__generic<T : __BuiltinType, let N : int, let M : int> bool all(matrix<T,N,M> x);
 
 // Barrier for writes to all memory spaces (HLSL SM 5.0)
 void AllMemoryBarrier();
@@ -250,9 +250,9 @@ void AllMemoryBarrier();
 void AllMemoryBarrierWithGroupSync();
 
 // Test if any components is non-zero (HLSL SM 1.0)
-__generic<T : __BuiltinType> T any(T x);
-__generic<T : __BuiltinType, let N : int> vector<T,N> any(vector<T,N> x);
-__generic<T : __BuiltinType, let N : int, let M : int> matrix<T,N,M> any(matrix<T,N,M> x);
+__generic<T : __BuiltinType> bool any(T x);
+__generic<T : __BuiltinType, let N : int> bool any(vector<T,N> x);
+__generic<T : __BuiltinType, let N : int, let M : int> bool any(matrix<T,N,M> x);
 
 
 // Reinterpret bits as a double (HLSL SM 5.0)
diff --git a/source/slang/hlsl.meta.slang.h b/source/slang/hlsl.meta.slang.h
index 2f42e9382..fd245d840 100644
--- a/source/slang/hlsl.meta.slang.h
+++ b/source/slang/hlsl.meta.slang.h
@@ -241,9 +241,9 @@ sb << "__generic<T : __BuiltinFloatingPointType, let N : int> vector<T,N> acos(v
 sb << "__generic<T : __BuiltinFloatingPointType, let N : int, let M : int> matrix<T,N,M> acos(matrix<T,N,M> x);\n";
 sb << "\n";
 sb << "// Test if all components are non-zero (HLSL SM 1.0)\n";
-sb << "__generic<T : __BuiltinType> T all(T x);\n";
-sb << "__generic<T : __BuiltinType, let N : int> vector<T,N> all(vector<T,N> x);\n";
-sb << "__generic<T : __BuiltinType, let N : int, let M : int> matrix<T,N,M> all(matrix<T,N,M> x);\n";
+sb << "__generic<T : __BuiltinType> bool all(T x);\n";
+sb << "__generic<T : __BuiltinType, let N : int> bool all(vector<T,N> x);\n";
+sb << "__generic<T : __BuiltinType, let N : int, let M : int> bool all(matrix<T,N,M> x);\n";
 sb << "\n";
 sb << "// Barrier for writes to all memory spaces (HLSL SM 5.0)\n";
 sb << "void AllMemoryBarrier();\n";
@@ -252,9 +252,9 @@ sb << "// Thread-group sync and barrier for writes to all memory spaces (HLSL SM
 sb << "void AllMemoryBarrierWithGroupSync();\n";
 sb << "\n";
 sb << "// Test if any components is non-zero (HLSL SM 1.0)\n";
-sb << "__generic<T : __BuiltinType> T any(T x);\n";
-sb << "__generic<T : __BuiltinType, let N : int> vector<T,N> any(vector<T,N> x);\n";
-sb << "__generic<T : __BuiltinType, let N : int, let M : int> matrix<T,N,M> any(matrix<T,N,M> x);\n";
+sb << "__generic<T : __BuiltinType> bool any(T x);\n";
+sb << "__generic<T : __BuiltinType, let N : int> bool any(vector<T,N> x);\n";
+sb << "__generic<T : __BuiltinType, let N : int, let M : int> bool any(matrix<T,N,M> x);\n";
 sb << "\n";
 sb << "\n";
 sb << "// Reinterpret bits as a double (HLSL SM 5.0)\n";
diff --git a/source/slang/ir-inst-defs.h b/source/slang/ir-inst-defs.h
index 38515e6f0..41e614807 100644
--- a/source/slang/ir-inst-defs.h
+++ b/source/slang/ir-inst-defs.h
@@ -175,16 +175,19 @@ INST(swizzleSet, swizzleSet, 2, 0)
 INST(ReturnVal, return_val, 1, 0)
 INST(ReturnVoid, return_void, 1, 0)
 
+// unconditionalBranch <target>
 INST(unconditionalBranch, unconditionalBranch, 1, 0)
-INST(break, break, 1, 0)
-INST(continue, continue, 1, 0)
+
+// loop <target> <breakLabel> <continueLabel>
 INST(loop, loop, 3, 0)
 
-INST(conditionalBranch, conditionalBranch, 1, 0)
-INST(if, if, 3, 0)
+// conditionalBranch <condition> <trueBlock> <falseBlock>
+INST(conditionalBranch, conditionalBranch, 3, 0)
+
+// ifElse <condition> <trueBlock> <falseBlock> <mergeBlock>
 INST(ifElse, ifElse, 4, 0)
-INST(loopTest, loopTest, 3, 0)
 
+// switch <val> <break> <default> <caseVal1> <caseBlock1> ...
 INST(switch, switch, 3, 0)
 
 INST(discard, discard, 0, 0)
diff --git a/source/slang/ir-ssa.cpp b/source/slang/ir-ssa.cpp
index 366ecc279..8a2d201dd 100644
--- a/source/slang/ir-ssa.cpp
+++ b/source/slang/ir-ssa.cpp
@@ -28,9 +28,9 @@ struct PhiInfo : RefObject
     // order in which the predecessor blocks get enumerated.
     List<IRUse> operands;
 
-    // Did we end up removing this phi because it was determined
-    // to be trivial?
-    bool wasRemoved = false;
+    // If this phi ended up being removed as trivial, then
+    // this will be the value that we replaced it with.
+    IRValue* replacement = nullptr;
 };
 
 // Information about a basic block that we generate/use
@@ -270,12 +270,16 @@ IRValue* tryRemoveTrivialPhi(
     // of itself) with the unique non-phi value.
     phi->replaceUsesWith(same);
 
-    // We will mark the phi as removed in the `PhiInfo` structure,
-    // so that we can avoid adding it to the block later.
-    phiInfo->wasRemoved = true;
+    // Clear out the operands to the phi, since they won't
+    // actually get used in the program any more.
+    for( auto& u : phiInfo->operands )
+    {
+        u.clear();
+    }
 
-    // TODO: we need a way to record that this parameter
-    // is no longer to be used...
+    // We will record the value that was used to replace this
+    // phi, so that we can easily look it up later.
+    phiInfo->replacement = same;
 
     // TODO: need to recursively consider chances to simplify
     // other phi nodes now.
@@ -476,6 +480,29 @@ IRValue* 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;
+        }
+
+
         return val;
     }
 
@@ -724,9 +751,9 @@ void constructSSA(ConstructSSAContext* context)
 
         for (auto phiInfo : blockInfo->phis)
         {
-            // If we eliminated this phi, then we had better not
-            // include it in the result.
-            if (phiInfo->wasRemoved)
+            // If we replaced this phi with another value,
+            // then we had better not include it in the result.
+            if (phiInfo->replacement)
                 continue;
 
             // We should add the phi as an explicit parameter of
diff --git a/source/slang/ir.cpp b/source/slang/ir.cpp
index 634632f0d..824af64ae 100644
--- a/source/slang/ir.cpp
+++ b/source/slang/ir.cpp
@@ -170,8 +170,6 @@ namespace Slang
             break;
 
         case kIROp_unconditionalBranch:
-        case kIROp_break:
-        case kIROp_continue:
         case kIROp_loop:
             // unconditonalBranch <block>
             begin = args + 0;
@@ -179,9 +177,7 @@ namespace Slang
             break;
 
         case kIROp_conditionalBranch:
-        case kIROp_if:
         case kIROp_ifElse:
-        case kIROp_loopTest:
             // conditionalBranch <condition> <trueBlock> <falseBlock>
             begin = args + 1;
             end = begin + 2;
@@ -401,12 +397,8 @@ namespace Slang
         case kIROp_ReturnVoid:
         case kIROp_unconditionalBranch:
         case kIROp_conditionalBranch:
-        case kIROp_break:
-        case kIROp_continue:
         case kIROp_loop:
-        case kIROp_if:
         case kIROp_ifElse:
-        case kIROp_loopTest:
         case kIROp_discard:
         case kIROp_switch:
         case kIROp_unreachable:
@@ -1302,6 +1294,15 @@ namespace Slang
             return nullptr;
         }
 
+        // Ugly special case: the result of loading from `groupshared`
+        // memory should not itself be `groupshared`.
+        //
+        // TODO: Should this generalize to any "rate-qualified" type?
+        if(auto rateType = valueType->As<GroupSharedType>())
+        {
+            valueType = rateType->valueType;
+        }
+
         auto inst = createInst<IRLoad>(
             this,
             kIROp_Load,
@@ -1526,25 +1527,13 @@ namespace Slang
     IRInst* IRBuilder::emitBreak(
         IRBlock*    target)
     {
-        auto inst = createInst<IRBreak>(
-            this,
-            kIROp_break,
-            nullptr,
-            target);
-        addInst(inst);
-        return inst;
+        return emitBranch(target);
     }
 
     IRInst* IRBuilder::emitContinue(
         IRBlock*    target)
     {
-        auto inst = createInst<IRContinue>(
-            this,
-            kIROp_continue,
-            nullptr,
-            target);
-        addInst(inst);
-        return inst;
+        return emitBranch(target);
     }
 
     IRInst* IRBuilder::emitLoop(
@@ -1583,17 +1572,18 @@ namespace Slang
         return inst;
     }
 
-    IRInst* IRBuilder::emitIf(
+    IRInst* IRBuilder::emitIfElse(
         IRValue*    val,
         IRBlock*    trueBlock,
+        IRBlock*    falseBlock,
         IRBlock*    afterBlock)
     {
-        IRValue* args[] = { val, trueBlock, afterBlock };
+        IRValue* args[] = { val, trueBlock, falseBlock, afterBlock };
         UInt argCount = sizeof(args) / sizeof(args[0]);
 
-        auto inst = createInst<IRIf>(
+        auto inst = createInst<IRIfElse>(
             this,
-            kIROp_if,
+            kIROp_ifElse,
             nullptr,
             argCount,
             args);
@@ -1601,23 +1591,12 @@ namespace Slang
         return inst;
     }
 
-    IRInst* IRBuilder::emitIfElse(
+    IRInst* IRBuilder::emitIf(
         IRValue*    val,
         IRBlock*    trueBlock,
-        IRBlock*    falseBlock,
         IRBlock*    afterBlock)
     {
-        IRValue* args[] = { val, trueBlock, falseBlock, afterBlock };
-        UInt argCount = sizeof(args) / sizeof(args[0]);
-
-        auto inst = createInst<IRIfElse>(
-            this,
-            kIROp_ifElse,
-            nullptr,
-            argCount,
-            args);
-        addInst(inst);
-        return inst;
+        return emitIfElse(val, trueBlock, afterBlock, afterBlock);
     }
 
     IRInst* IRBuilder::emitLoopTest(
@@ -1625,17 +1604,7 @@ namespace Slang
         IRBlock*    bodyBlock,
         IRBlock*    breakBlock)
     {
-        IRValue* args[] = { val, bodyBlock, breakBlock };
-        UInt argCount = sizeof(args) / sizeof(args[0]);
-
-        auto inst = createInst<IRLoopTest>(
-            this,
-            kIROp_loopTest,
-            nullptr,
-            argCount,
-            args);
-        addInst(inst);
-        return inst;
+        return emitIfElse(val, bodyBlock, breakBlock, bodyBlock);
     }
 
     IRInst* IRBuilder::emitSwitch(
diff --git a/source/slang/slang.cpp b/source/slang/slang.cpp
index c25a4f2ce..a918fa356 100644
--- a/source/slang/slang.cpp
+++ b/source/slang/slang.cpp
@@ -529,9 +529,16 @@ RefPtr<ModuleDecl> CompileRequest::loadModule(
 
     translationUnit->sourceFiles.Add(sourceFile);
 
+
+    int errorCountBefore = mSink.GetErrorCount();
     parseTranslationUnit(translationUnit.Ptr());
+    int errorCountAfter = mSink.GetErrorCount();
 
-    // TODO: handle errors
+    if( errorCountAfter != errorCountBefore )
+    {
+        // Something went wrong during the parsing, so we should bail out.
+        return nullptr;
+    }
 
     loadParsedModule(
         translationUnit,
diff --git a/source/slang/syntax.cpp b/source/slang/syntax.cpp
index 0428246db..d83a91938 100644
--- a/source/slang/syntax.cpp
+++ b/source/slang/syntax.cpp
@@ -45,6 +45,12 @@ namespace Slang
         case Slang::BaseType::Float:
             res.Append("float");
             break;
+        case Slang::BaseType::Double:
+            res.Append("double");
+            break;
+        case Slang::BaseType::Half:
+            res.Append("half");
+            break;
         case Slang::BaseType::Void:
             res.Append("void");
             break;
diff --git a/source/slang/vm.cpp b/source/slang/vm.cpp
index c486615c9..e910870ba 100644
--- a/source/slang/vm.cpp
+++ b/source/slang/vm.cpp
@@ -995,8 +995,6 @@ void resumeThread(
 
         case kIROp_loop:
         case kIROp_unconditionalBranch:
-        case kIROp_break:
-        case kIROp_continue:
             {
                 // For now our encoding is very regular, so we can decode without
                 // knowing too much about an instruction...
@@ -1045,8 +1043,6 @@ void resumeThread(
             }
             break;
 
-        case kIROp_loopTest:
-        case kIROp_if:
         case kIROp_ifElse:
         case kIROp_conditionalBranch:
             {