Add a warning on missing return, and initial SCCP pass (#671)

* Add a warning on missing return, and initial SCCP pass

The user-visible feature added here is a diagnostic for functions with non-`void` return type where control flow might fall off the end. This *sounds* like a trivial diagnostic to add as part of the front-end AST checking, but that can run afoul of really basic stuff like:

```hlsl
int thisFunctionisOkay(int a)
{
    while(true)
    {
        if(a > 10) return a;
        a = a*2 + 1;
    }
    // no return here!
}
```

This function "obviously" doesn't need to have a `return` statement at the end there, but realizing this fact relies on the compiler to understand that the `while(true)` loop can't exit normally, and doesn't contain any `break` statement. One can write "obvious" examples that need more and more complex analysis to rule out.

The answer Slang uses for stuff like this is to do the analysis at the IR level right after initial code generation (this would be before serialization, BTW, so that attached `IRHighLevelDeclDecoration`s can be used).

When lowering the AST to the IR, we always emit a `missingReturn` instruction (a subtype of `IRUnreachable`) at the end of its body if it isn't already terminated. The IR analysis pass to detect missing `return` statements is then as simple as just walking through all the functions in the module and making sure they don't contain `missingReturn` instructions.

For that simple pass to work, we first need to make some effort to remove dead blocks that control flow can never reach. This change adds a very basic initial implementation of Spare Conditional Constant Propagation (SCCP), which is a well-known SSA optimization that combines constant propagation over SSA form with dead code elimination over a CFG to achieve optimizations that are not possible with either optimization along.

For the moment, we don't actually implement any constant *folding* as part of the SCCP pass, so we can eliminate the dead block in a case like the function above (and those in the test case added in this change), but will not catch things like a `while(0 < 1)` loop. Handling more "obvious" cases like that is left for future work.

* fixup: warning on unreachable code

* Handle case where user of an inst isn't in same function/code

The code as assuming any instruction in the SSA work list has to come from the function/code being processed, but this misses the case where an instruction in a generic has a use inside the function that the generic produces.

This change adds code to guard against that case.

source/core/core.vcxproj

@@ -213,7 +213,7 @@
     <ClCompile Include="token-reader.cpp" />
   </ItemGroup>
   <ItemGroup>
-    <Natvis Include="core.natvis" />
+    <None Include="core.natvis" />
   </ItemGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">

source/core/core.vcxproj.filters

@@ -130,8 +130,8 @@
     </ClCompile>
   </ItemGroup>
   <ItemGroup>
-    <Natvis Include="core.natvis">
+    <None Include="core.natvis">
       <Filter>Source Files</Filter>
-    </Natvis>
+    </None>
   </ItemGroup>
 </Project>

source/slang/bytecode.cpp

@@ -409,7 +409,7 @@ void generateBytecodeForInst(
         }
         break;
 
-    case kIROp_boolConst:
+    case kIROp_BoolLit:
         {
             auto ii = (IRConstant*) inst;
             encodeUInt(context, ii->op);

source/slang/diagnostic-defs.h

@@ -364,6 +364,7 @@ DIAGNOSTIC(40008, Error, invalidLValueForRefParameter, "the form of this l-value
 
 DIAGNOSTIC(41000, Warning, unreachableCode, "unreachable code detected")
 
+DIAGNOSTIC(41010, Warning, missingReturn, "control flow may reach end of non-'void' function")
 
 //
 // 5xxxx - Target code generation.

source/slang/emit.cpp

@@ -2308,7 +2308,7 @@ struct EmitVisitor
             Emit(((IRConstant*) inst)->value.floatVal);
             break;
 
-        case kIROp_boolConst:
+        case kIROp_BoolLit:
             {
                 bool val = ((IRConstant*)inst)->value.intVal != 0;
                 emit(val ? "true" : "false");
@@ -2357,7 +2357,7 @@ struct EmitVisitor
         //
         case kIROp_IntLit:
         case kIROp_FloatLit:
-        case kIROp_boolConst:
+        case kIROp_BoolLit:
             return true;
 
         // Always fold these in, because their results
@@ -3484,7 +3484,7 @@ struct EmitVisitor
         {
         case kIROp_IntLit:
         case kIROp_FloatLit:
-        case kIROp_boolConst:
+        case kIROp_BoolLit:
             emitIRSimpleValue(ctx, inst);
             break;
 

source/slang/ir-constexpr.cpp

@@ -48,7 +48,7 @@ bool isConstExpr(IRInst* value)
     {
     case kIROp_IntLit:
     case kIROp_FloatLit:
-    case kIROp_boolConst:
+    case kIROp_BoolLit:
     case kIROp_Func:
         return true;
 
@@ -68,7 +68,7 @@ bool opCanBeConstExpr(IROp op)
     {
     case kIROp_IntLit:
     case kIROp_FloatLit:
-    case kIROp_boolConst:
+    case kIROp_BoolLit:
     case kIROp_Add:
     case kIROp_Sub:
     case kIROp_Mul:

source/slang/ir-inst-defs.h

@@ -183,11 +183,11 @@ INST_RANGE(Type, VoidType, StructType)
 INST_RANGE(ParentInst, StructType, Block)
 
 /* IRConstant */
-    INST(boolConst, boolConst, 0, 0)
+    INST(BoolLit, boolConst, 0, 0)
     INST(IntLit, integer_constant, 0, 0)
     INST(FloatLit, float_constant, 0, 0)
     INST(StringLit, string_constant, 0, 0)
-INST_RANGE(Constant, boolConst, StringLit)
+INST_RANGE(Constant, BoolLit, StringLit)
 
 INST(undefined, undefined, 0, 0)
 
@@ -294,25 +294,34 @@ INST(SwizzledStore, swizzledStore, 2, 0)
     INST(ReturnVal, return_val, 1, 0)
     INST(ReturnVoid, return_void, 1, 0)
 
-    // unconditionalBranch <target>
-    INST(unconditionalBranch, unconditionalBranch, 1, 0)
+    /* IRUnconditionalBranch */
+        // unconditionalBranch <target>
+        INST(unconditionalBranch, unconditionalBranch, 1, 0)
 
-    // loop <target> <breakLabel> <continueLabel>
-    INST(loop, loop, 3, 0)
+        // loop <target> <breakLabel> <continueLabel>
+        INST(loop, loop, 3, 0)
+    INST_RANGE(UnconditionalBranch, unconditionalBranch, loop)
 
-    // conditionalBranch <condition> <trueBlock> <falseBlock>
-    INST(conditionalBranch, conditionalBranch, 3, 0)
+    /* IRConditionalbranch */
 
-    // ifElse <condition> <trueBlock> <falseBlock> <mergeBlock>
-    INST(ifElse, ifElse, 4, 0)
+        // conditionalBranch <condition> <trueBlock> <falseBlock>
+        INST(conditionalBranch, conditionalBranch, 3, 0)
+
+        // ifElse <condition> <trueBlock> <falseBlock> <mergeBlock>
+        INST(ifElse, ifElse, 4, 0)
+    INST_RANGE(ConditionalBranch, conditionalBranch, ifElse)
 
     // switch <val> <break> <default> <caseVal1> <caseBlock1> ...
-    INST(switch, switch, 3, 0)
+    INST(Switch, switch, 3, 0)
 
     INST(discard, discard, 0, 0)
-    INST(unreachable, unreachable, 0, 0)
 
-INST_RANGE(TerminatorInst, ReturnVal, unreachable)
+    /* IRUnreachable */
+        INST(MissingReturn, missingReturn, 0, 0)
+        INST(Unreachable, unreachable, 0, 0)
+    INST_RANGE(Unreachable, MissingReturn, Unreachable)
+
+INST_RANGE(TerminatorInst, ReturnVal, Unreachable)
 
 INST(Add, add, 2, 0)
 INST(Sub, sub, 2, 0)

source/slang/ir-insts.h

@@ -227,7 +227,14 @@ struct IRDiscard : IRTerminatorInst
 // that a block ending in one of these can actually be
 // executed.
 struct IRUnreachable : IRTerminatorInst
-{};
+{
+    IR_PARENT_ISA(Unreachable);
+};
+
+struct IRMissingReturn : IRUnreachable
+{
+    IR_LEAF_ISA(MissingReturn);
+};
 
 struct IRBlock;
 
@@ -236,6 +243,12 @@ struct IRUnconditionalBranch : IRTerminatorInst
     IRUse block;
 
     IRBlock* getTargetBlock() { return (IRBlock*)block.get(); }
+
+    UInt getArgCount();
+    IRUse* getArgs();
+    IRInst* getArg(UInt index);
+
+    IR_PARENT_ISA(UnconditionalBranch);
 };
 
 // Special cases of unconditional branch, to handle
@@ -264,6 +277,8 @@ struct IRLoop : IRUnconditionalBranch
 
 struct IRConditionalBranch : IRTerminatorInst
 {
+    IR_PARENT_ISA(ConditionalBranch)
+
     IRUse condition;
     IRUse trueBlock;
     IRUse falseBlock;
@@ -303,6 +318,8 @@ struct IRIfElse : IRConditionalBranch
 // A multi-way branch that represents a source-level `switch`
 struct IRSwitch : IRTerminatorInst
 {
+    IR_LEAF_ISA(Switch);
+
     IRUse condition;
     IRUse breakLabel;
     IRUse defaultLabel;
@@ -772,6 +789,7 @@ struct IRBuilder
     IRInst* emitDiscard();
 
     IRInst* emitUnreachable();
+    IRInst* emitMissingReturn();
 
     IRInst* emitBranch(
         IRBlock*    block);

source/slang/ir-missing-return.cpp

@@ -0,0 +1,46 @@
+// ir-missing-return.cpp
+#include "ir-missing-return.h"
+
+#include "ir.h"
+#include "ir-insts.h"
+
+namespace Slang {
+
+class DiagnosticSink;
+struct IRModule;
+
+void checkForMissingReturnsRec(
+    IRInst*         inst,
+    DiagnosticSink* sink)
+{
+    if( auto code = as<IRGlobalValueWithCode>(inst) )
+    {
+        for( auto block : code->getBlocks() )
+        {
+            auto terminator = block->getTerminator();
+
+            if( auto missingReturn = as<IRMissingReturn>(terminator) )
+            {
+                sink->diagnose(missingReturn, Diagnostics::missingReturn);
+            }
+        }
+    }
+
+    if( auto parentInst = as<IRParentInst>(inst) )
+    {
+        for( auto childInst : parentInst->getChildren() )
+        {
+            checkForMissingReturnsRec(childInst, sink);
+        }
+    }
+}
+
+void checkForMissingReturns(
+    IRModule*       module,
+    DiagnosticSink* sink)
+{
+    // Look for any `missingReturn` instructions
+    checkForMissingReturnsRec(module->getModuleInst(), sink);
+}
+
+}

source/slang/ir-missing-return.h

@@ -0,0 +1,12 @@
+// ir-missing-return.h
+#pragma once
+
+namespace Slang
+{
+    class DiagnosticSink;
+    struct IRModule;
+
+    void checkForMissingReturns(
+        IRModule*       module,
+        DiagnosticSink* sink);
+}

source/slang/ir-restructure.cpp

@@ -251,7 +251,8 @@ namespace Slang
                 //
                 SLANG_UNEXPECTED("unhandled terminator instruction opcode");
                 // fall through to:
-            case kIROp_unreachable:
+            case kIROp_Unreachable:
+            case kIROp_MissingReturn:
             case kIROp_ReturnVal:
             case kIROp_ReturnVoid:
             case kIROp_discard:
@@ -446,7 +447,7 @@ namespace Slang
                 }
                 break;
 
-            case kIROp_switch:
+            case kIROp_Switch:
                 {
                     // A `switch` instruction will always translate
                     // to a `SwitchRegion` and then to a `switch` statement.