Warn when inout parameter is never written (#4777)

Addresses #4698 as one approach to diagnose the potential problem.

Emit warnings when a user marks a parameter as `inout` but never writes to it in the function. A new intrinsic function `unmodified(out T)` has been added to explicitly indicate that an `inout` variable will not be modified in the function.

This is only one way to address the specific validation error in #4698. In general it seems that DXC does some more extensive checks on actual struct fields (as opposed to observing arbitrary struct writes), so that will be the next step.

11 files changed, 222 insertions, 46 deletions

diff --git a/source/slang/core.meta.slang b/source/slang/core.meta.slang
index 2dae7b3c4..695423285 100644
--- a/source/slang/core.meta.slang
+++ b/source/slang/core.meta.slang
@@ -2025,13 +2025,20 @@ T reinterpret(U value);
 
 // Use an otherwise unused value
 //
-// This can be used to silence the warning about returning before initializing
-// an out paramter.
+// This can be used to silence the warning about returning before initializing an out paramter.
 __generic<T>
 [__readNone]
 [ForceInline]
+__intrinsic_op($(kIROp_Unmodified))
 void unused(inout T){}
 
+// This can be used to silence the warning about not writing to an inout parameter.
+__generic<T>
+[__readNone]
+[ForceInline]
+__intrinsic_op($(kIROp_Unmodified))
+void unmodified(out T){}
+
 // Specialized function
 
 /// Given a string returns an integer hash of that string.
diff --git a/source/slang/diff.meta.slang b/source/slang/diff.meta.slang
index c912e026c..a4c468ef7 100644
--- a/source/slang/diff.meta.slang
+++ b/source/slang/diff.meta.slang
@@ -273,7 +273,12 @@ struct TensorView
     __subscript(uint index) -> T
     {
         [ForceInline] [__NoSideEffect] get { return load(index); }
-        [ForceInline] set { store(index, newValue); }
+        
+        [ForceInline] set
+        {
+            unmodified(this);
+            store(index, newValue);
+        }
 
         [__NoSideEffect]
         ref
@@ -288,7 +293,13 @@ struct TensorView
     __subscript(uint i1, uint i2) -> T
     {
         [ForceInline] [__NoSideEffect] get { return load(i1, i2); }
-        [ForceInline] set { store(i1, i2, newValue); }
+
+        [ForceInline] set
+        {
+            unmodified(this);
+            store(i1, i2, newValue);
+        }
+        
         [__NoSideEffect]
         ref
         {
@@ -302,7 +313,13 @@ struct TensorView
     __subscript(uint2 i) -> T
     {
         [ForceInline] [__NoSideEffect] get { return load(i.x, i.y); }
-        [ForceInline] set { store(i.x, i.y, newValue); }
+
+        [ForceInline] set
+        {
+            unmodified(this);
+            store(i.x, i.y, newValue);
+        }
+
         [__NoSideEffect]
         ref
         {
@@ -316,7 +333,13 @@ struct TensorView
     __subscript(uint i1, uint i2, uint i3) -> T
     {
         [ForceInline] [__NoSideEffect] get { return load(i1, i2, i3); }
-        [ForceInline] set { store(i1, i2, i3, newValue); }
+        
+        [ForceInline] set
+        {
+            unmodified(this);
+            store(i1, i2, i3, newValue);
+        }
+
         [__NoSideEffect]
         ref
         {
@@ -330,7 +353,13 @@ struct TensorView
     __subscript(uint3 i) -> T
     {
         [ForceInline] [__NoSideEffect] get { return load(i.x, i.y, i.z); }
-        [ForceInline] set { store(i.x, i.y, i.z, newValue); }
+
+        [ForceInline] set
+        {
+            unmodified(this);
+            store(i.x, i.y, i.z, newValue);
+        }
+
         [__NoSideEffect]
         ref
         {
@@ -344,7 +373,13 @@ struct TensorView
     __subscript(uint i1, uint i2, uint i3, uint i4) -> T
     {
         [ForceInline] [__NoSideEffect] get { return load(i1, i2, i3, i4); }
-        [ForceInline] set { store(i1, i2, i3, i4, newValue); }
+
+        [ForceInline] set
+        {
+            unmodified(this);
+            store(i1, i2, i3, i4, newValue);
+        }
+
         [__NoSideEffect]
         ref
         {
@@ -358,7 +393,13 @@ struct TensorView
     __subscript(uint4 i) -> T
     {
         [__NoSideEffect][ForceInline] get { return load(i.x, i.y, i.z, i.w); }
-        [ForceInline] set { store(i.x, i.y, i.z, i.w, newValue); }
+
+        [ForceInline] set
+        {
+            unmodified(this);
+            store(i.x, i.y, i.z, i.w, newValue);
+        }
+
         [__NoSideEffect]
         ref
         {
@@ -372,7 +413,13 @@ struct TensorView
     __subscript(uint i1, uint i2, uint i3, uint i4, uint i5) -> T
     {
         [ForceInline] [__NoSideEffect] get { return load(i1, i2, i3, i4, i5); }
-        [ForceInline] set { store(i1, i2, i3, i4, i5, newValue); }
+        
+        [ForceInline] set
+        {
+            unmodified(this);
+            store(i1, i2, i3, i4, i5, newValue);
+        }
+
         [__NoSideEffect]
         ref
         {
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index 6d7be3f92..44e8aa13d 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -752,6 +752,8 @@ DIAGNOSTIC(41018, Warning, returningWithUninitializedOut, "returning without ini
 DIAGNOSTIC(41019, Warning, returningWithPartiallyUninitializedOut, "returning without fully initializing out parameter '$0'")
 DIAGNOSTIC(41020, Warning, constructorUninitializedField, "exiting constructor without initializing field '$0'")
 DIAGNOSTIC(41021, Warning, fieldNotDefaultInitialized, "default initializer for '$0' will not initialize field '$1'")
+DIAGNOSTIC(41022, Warning, inOutNeverStoredInto, "inout parameter '$0' is never written to")
+DIAGNOSTIC(41023, Warning, methodNeverMutates, "method marked `[mutable]` but never modifies `this`")
 
 DIAGNOSTIC(41011, Error, typeDoesNotFitAnyValueSize, "type '$0' does not fit in the size required by its conforming interface.")
 DIAGNOSTIC(41012, Note, typeAndLimit, "sizeof($0) is $1, limit is $2")
diff --git a/source/slang/slang-emit-c-like.cpp b/source/slang/slang-emit-c-like.cpp
index dd5ef8c51..fadf76dd2 100644
--- a/source/slang/slang-emit-c-like.cpp
+++ b/source/slang/slang-emit-c-like.cpp
@@ -2889,6 +2889,9 @@ void CLikeSourceEmitter::_emitInst(IRInst* inst)
     case kIROp_DebugValue:
         break;
 
+    case kIROp_Unmodified:
+        break;
+
         // Insts that needs to be emitted as code blocks.
     case kIROp_CudaKernelLaunch:
     case kIROp_AtomicCounterIncrement:
diff --git a/source/slang/slang-emit-spirv.cpp b/source/slang/slang-emit-spirv.cpp
index 23eaa4435..bb0a2565c 100644
--- a/source/slang/slang-emit-spirv.cpp
+++ b/source/slang/slang-emit-spirv.cpp
@@ -2719,6 +2719,7 @@ struct SPIRVEmitContext
         case kIROp_Specialize:
         case kIROp_MissingReturn:
         case kIROp_StaticAssert:
+        case kIROp_Unmodified:
             break;
         case kIROp_Var:
             result = emitVar(parent, inst);
diff --git a/source/slang/slang-ir-addr-inst-elimination.cpp b/source/slang/slang-ir-addr-inst-elimination.cpp
index 2581ea37f..7889a2f61 100644
--- a/source/slang/slang-ir-addr-inst-elimination.cpp
+++ b/source/slang/slang-ir-addr-inst-elimination.cpp
@@ -169,6 +169,7 @@ struct AddressInstEliminationContext
                     break;
                 case kIROp_GetElementPtr:
                 case kIROp_FieldAddress:
+                case kIROp_Unmodified:
                     break;
                 default:
                     sink->diagnose(use->getUser()->sourceLoc, Diagnostics::unsupportedUseOfLValueForAutoDiff);
diff --git a/source/slang/slang-ir-autodiff-transcriber-base.h b/source/slang/slang-ir-autodiff-transcriber-base.h
index 7b4c293e9..f672631e3 100644
--- a/source/slang/slang-ir-autodiff-transcriber-base.h
+++ b/source/slang/slang-ir-autodiff-transcriber-base.h
@@ -123,8 +123,14 @@ struct AutoDiffTranscriberBase
 
     InstPair transcribeBlock(IRBuilder* builder, IRBlock* origBlock)
     {
-        HashSet<IRInst*> emptySet;
-        return transcribeBlockImpl(builder, origBlock, emptySet);
+        HashSet<IRInst*> ignore;
+        for (auto inst = origBlock->getFirstInst(); inst; inst = inst->next)
+        {
+            if (inst->m_op == kIROp_Unmodified)
+                ignore.add(inst);
+        }
+
+        return transcribeBlockImpl(builder, origBlock, ignore);
     }
 
     // Transcribe a generic definition
diff --git a/source/slang/slang-ir-inst-defs.h b/source/slang/slang-ir-inst-defs.h
index 0e84be2b2..84ee634a1 100644
--- a/source/slang/slang-ir-inst-defs.h
+++ b/source/slang/slang-ir-inst-defs.h
@@ -931,6 +931,7 @@ INST_RANGE(BindingQuery, GetRegisterIndex, GetRegisterSpace)
 
     INST(SemanticDecoration, semantic, 2, 0)
     INST(ConstructorDecoration, constructor, 1, 0)
+    INST(MethodDecoration, method, 0, 0)
     INST(PackOffsetDecoration, packoffset, 2, 0)
 
         // Reflection metadata for a shader parameter that provides the original type name.
@@ -1094,6 +1095,7 @@ INST(ExtractTaggedUnionPayload,         extractTaggedUnionPayload,  1, 0)
 
 INST(BitCast,                           bitCast,                    1, 0)
 INST(Reinterpret,                       reinterpret,                1, 0)
+INST(Unmodified,                        unmodified,                1, 0)
 INST(OutImplicitCast,                   outImplicitCast,           1, 0)
 INST(InOutImplicitCast,                 inOutImplicitCast,         1, 0)
 INST(IntCast, intCast, 1, 0)
diff --git a/source/slang/slang-ir-insts.h b/source/slang/slang-ir-insts.h
index 8a801e4e7..e30b903b5 100644
--- a/source/slang/slang-ir-insts.h
+++ b/source/slang/slang-ir-insts.h
@@ -1365,6 +1365,8 @@ struct IRConstructorDecorartion : IRDecoration
     bool getSynthesizedStatus() { return cast<IRBoolLit>(getOperand(0))->getValue(); }
 };
 
+IR_SIMPLE_DECORATION(MethodDecoration)
+
 struct IRPackOffsetDecoration : IRDecoration
 {
     enum
diff --git a/source/slang/slang-ir-use-uninitialized-values.cpp b/source/slang/slang-ir-use-uninitialized-values.cpp
index ca723a97c..b8dfcc33c 100644
--- a/source/slang/slang-ir-use-uninitialized-values.cpp
+++ b/source/slang/slang-ir-use-uninitialized-values.cpp
@@ -38,30 +38,67 @@ namespace Slang
             || (inst->m_op == kIROp_Var);
     }
 
-    static bool isPotentiallyUnintended(IRParam* param)
+    static bool isUnmodifying(IRFunc *func)
     {
-        auto outType = as<IROutType>(param->getFullType());
-        if (!outType)
-            return false;
-
-        // Don't check `out Vertices<T>` or `out Indices<T>` parameters
-        // in mesh shaders.
-        // TODO: we should find a better way to represent these mesh shader
-        // parameters so they conform to the initialize before use convention.
-        // For example, we can use a `OutputVetices` and `OutputIndices` type
-        // to represent an output, like `OutputPatch` in domain shader.
-        // For now, we just skip the check for these parameters.
-        switch (outType->getValueType()->getOp())
-        {
-        case kIROp_VerticesType:
-        case kIROp_IndicesType:
-        case kIROp_PrimitivesType:
-            return false;
-        default:
-            break;
+        auto intr = func->findDecoration<IRIntrinsicOpDecoration>();
+        return (intr && intr->getIntrinsicOp() == kIROp_Unmodified);
+    }
+
+    enum ParameterCheckType
+    {
+        Never,    // Parameter does NOT to be checked for uninitialization (e.g. is `in` or special type)
+        AsOut,    // Parameter DOES need to be checked for usage before initializations
+        AsInOut   // Parameter DOES need to be checked to see if it is ever written to
+    };
+
+    static ParameterCheckType isPotentiallyUnintended(IRParam* param, Stage stage, int index)
+    {
+        IRType* type = param->getFullType();
+        if (auto out = as<IROutType>(param->getFullType()))
+        {
+            // Don't check `out Vertices<T>` or `out Indices<T>` parameters
+            // in mesh shaders.
+            // TODO: we should find a better way to represent these mesh shader
+            // parameters so they conform to the initialize before use convention.
+            // For example, we can use a `OutputVetices` and `OutputIndices` type
+            // to represent an output, like `OutputPatch` in domain shader.
+            // For now, we just skip the check for these parameters.
+            switch (out->getValueType()->getOp())
+            {
+            case kIROp_VerticesType:
+            case kIROp_IndicesType:
+            case kIROp_PrimitivesType:
+                return Never;
+            default:
+                break;
+            }
+
+            return AsOut;
+        }
+        else if (auto inout = as<IRInOutType>(type))
+        {
+            // TODO: some way to check if the method
+            // is actually used for autodiff
+            if (as<IRDifferentialPairUserCodeType>(inout->getValueType()))
+                return Never;
+
+            switch (stage)
+            {
+            case Stage::AnyHit:
+            case Stage::ClosestHit:
+                // In HLSL the payload is required to be `inout`
+                return (index == 0) ? Never : AsInOut;
+            case Stage::Geometry:
+                // Second parameter is the triangle stream
+                return (index == 1) ? Never : AsInOut;
+            default:
+                break;
+            }
+
+            return AsInOut;
         }
 
-        return true;
+        return Never;
     }
 
     static bool isAliasable(IRInst* inst)
@@ -275,6 +312,7 @@ namespace Slang
         
         // Miscellaenous cases
         case kIROp_ManagedPtrAttach:
+        case kIROp_Unmodified:
             stores.add(user);
             break;
 
@@ -481,6 +519,62 @@ namespace Slang
         }
     }
 
+    static void checkParameterAsOut(ReachabilityContext &reachability, IRFunc* func, IRParam* param, DiagnosticSink* sink)
+    {
+        auto loads = getUnresolvedParamLoads(reachability, func, param);
+        for (auto load : loads)
+        {
+            sink->diagnose(load,
+                as<IRTerminatorInst>(load)
+                ? Diagnostics::returningWithUninitializedOut
+                : Diagnostics::usingUninitializedOut,
+                param);
+        }
+    }
+
+    static void checkParameterAsInOut(IRParam* param, IRFunc* func, bool isThis, DiagnosticSink* sink)
+    {
+        // If the inout is used for the sake of interface conformance, let it be
+        for (auto use = func->firstUse; use; use = use->nextUse)
+        {
+            if (as<IRWitnessTableEntry>(use->getUser()))
+                return;
+        }
+
+        // If there is at least one write...
+        List<IRInst*> stores;
+        List<IRInst*> loads;
+
+        for (auto alias : getAliasableInstructions(param))
+        {
+            for (auto use = alias->firstUse; use; use = use->nextUse)
+            {
+                IRInst* user = use->getUser();
+                collectLoadStore(stores, loads, user, alias);
+
+                // ...we will ignore the rest...
+                if (stores.getCount())
+                    return;
+            }
+        }
+
+        // ...or if there is an intrinsic_asm instruction
+        for (const auto& b : func->getBlocks())
+        {
+            for (auto inst = b->getFirstInst(); inst; inst = inst->next)
+            {
+                if (as<IRGenericAsm>(inst))
+                    return;
+            }
+        }
+
+        sink->diagnose(param,
+            isThis
+            ? Diagnostics::methodNeverMutates
+            : Diagnostics::inOutNeverStoredInto,
+            param);
+    }
+
     static void checkUninitializedValues(IRFunc* func, DiagnosticSink* sink)
     {
         // Differentiable functions will generate undefined values
@@ -495,22 +589,30 @@ namespace Slang
         ReachabilityContext reachability(func);
 
         // Used for a further analysis and to skip usual return checks
-        auto constructor = func->findDecoration <IRConstructorDecorartion> ();
+        auto constructor = func->findDecoration<IRConstructorDecorartion>();
+
+        // Special checks for stages e.g. raytracing shader
+        Stage stage = Stage::Unknown;
+        if (auto entry = func->findDecoration<IREntryPointDecoration>())
+            stage = entry->getProfile().getStage();
+
+        bool structMethod = func->findDecoration<IRMethodDecoration>();
 
         // Check out parameters
-        for (auto param : firstBlock->getParams())
+        if (!isUnmodifying(func))
         {
-            if (!isPotentiallyUnintended(param))
-                continue;
-
-            auto loads = getUnresolvedParamLoads(reachability, func, param);
-            for (auto load : loads)
+            int index = 0;
+            for (auto param : firstBlock->getParams())
             {
-                sink->diagnose(load,
-                        as<IRTerminatorInst>(load)
-                        ? Diagnostics::returningWithUninitializedOut
-                        : Diagnostics::usingUninitializedOut,
-                        param);
+                bool isThis = structMethod && (index == 0);
+
+                ParameterCheckType checkType = isPotentiallyUnintended(param, stage, index);
+                if (checkType == AsOut)
+                    checkParameterAsOut(reachability, func, param, sink);
+                else if (checkType == AsInOut)
+                    checkParameterAsInOut(param, func, isThis, sink);
+
+                index++;
             }
         }
 
diff --git a/source/slang/slang-lower-to-ir.cpp b/source/slang/slang-lower-to-ir.cpp
index 66918c2f1..50d017fb9 100644
--- a/source/slang/slang-lower-to-ir.cpp
+++ b/source/slang/slang-lower-to-ir.cpp
@@ -9509,7 +9509,6 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
 
     LoweredValInfo lowerFuncDeclInContext(IRGenContext* subContext, IRBuilder* subBuilder, FunctionDeclBase* decl, bool emitBody = true)
     {
-
         IRGeneric* outerGeneric = nullptr;
         subContext->funcDecl = decl;
 
@@ -9544,6 +9543,10 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
             }
         }
 
+        // For diagnostics
+        if (as<StructDecl>(decl->parentDecl))
+            getBuilder()->addSimpleDecoration<IRMethodDecoration>(irFunc);
+
         auto irFuncType = info.type;
         auto& irResultType = info.resultType;
         auto& parameterLists = info.parameterLists;