Cleanups around around parameter-passing modes (#8754)

The primary goal of this change is to try to fix (read: *remove*) the
logic that ovewrites the parameter-passing mode of an `out` parameter of
non-copyable type to be a `ref` parameter instead. I need that change in
order to implement a more reasonable pass to check for use of
uninitialized values in IR, because the semantics of an `out` parameter
are quite different from a `ref`, and I need to be able to tell them
apart.

Trying to make a systematic fix for the issue led me to make some more
broad refactoring and cleanup changes to operations that deal with
parameter-passing modes. Most notably, I have cleaned up several key
functions in `slang-lower-to-ir.cpp` to have more clear comments that
explain what they are doing, and why.

The general idea is that determining the parameter-passing mode of a
parameter breaks down into a few phases:

* First, theres the "nominal" (aka "declared") parameter-passing mode.
This is either the mode that is explicitly indicated by modifiers in the
source code (e.g., `in` or `out` on an explicit parameter declaration,
or `[mutating]` to control the implicit `this` parameter of a method),
or a mode that is inferred from other context related to the declaration
(e.g., the implicit `this` parameter of a `set` accessor defaults to
`inout`, but an implicit `this` in the context of a `class` is always
`in`).

* Next there is the "actual" parameter passing mode, which is based on
the nominal mode, but may be adjusted based on the type of the
parameter. For example, a parameter that is declared as `in` but that
has a non-copyable type will be treated as `borrow in`. There's an
additional wrinkled related to how we handle entry-point varying input
parameters (another case where a parameter declared as `in` may be
translated to use `borrow in`).

One of my goals was to try to bottleneck the logic for both
explicitly-declared parameters and the implicit `this` parameter through
the same code path, at least for computing the actual parameter-passing
mode from the nominal mode and parameter type. I don't think the logic
is as clean as it can/should be yet, but this is an incremental step in
a better direction.

The most important concern I have about this change is whether it will
break any existing code, because I have changed cases where a parameter
of non-copyable type would previously have been modified at the AST
level to use `ref` over to use `inout` or `out` instead. It is
reasonable to be concerned that this change could result in IR code
attempting to copy values of non-copyable type, which could break code
that uses non-copyable types like `RayQuery` and `HitObject`. I note
that the compiler already allows parameters of non-copyable type as
`borrow in` parameters (which allow implicit copies to be introduced,
just as for `inout` or `out`), so if the handling of `out` and `inout`
for non-copyable types is broken, it would strongly imply that `borrow
in` parameters of non-copyable types are already broken.

If it turns out that there is no reasonable way to stop passes on Slang
IR from introducing copies of non-copyable types when they use `out` or
`inout` (or even `borrow in`), then it seems like we will need to
introduce even more fine-grained parameter passing modes, to reflect the
difference between what we might call "value" borrows and "memory"
borrows, where the former allow copies to be introduced and the latter
don't. I really hope I don't have to go down that road, though, since it
is already challenging to explain to folks what all the existing
parameter-passing modes mean.

---------

Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>
Co-authored-by: James Helferty (NVIDIA) <jhelferty@nvidia.com>

Author: 3 people

source/slang/slang-ast-type.cpp

@@ -610,15 +610,15 @@ ParamPassingMode getParamPassingModeFromPossiblyWrappedParamType(Type* paramType
     }
 }
 
-ParamPassingMode FuncType::getParamDirection(Index index)
+ParamPassingMode FuncType::getParamPassingMode(Index index)
 {
-    auto paramType = getParamTypeWithDirectionWrapper(index);
+    auto paramType = getParamTypeWithModeWrapper(index);
     return getParamPassingModeFromPossiblyWrappedParamType(paramType);
 }
 
 Type* FuncType::getParamValueType(Index index)
 {
-    auto paramType = getParamTypeWithDirectionWrapper(index);
+    auto paramType = getParamTypeWithModeWrapper(index);
     if (auto wrappedParamType = as<ParamPassingModeType>(paramType))
         return wrappedParamType->getValueType();
     return paramType;
@@ -635,7 +635,7 @@ void FuncType::_toTextOverride(StringBuilder& out)
         {
             out << toSlice(", ");
         }
-        out << getParamTypeWithDirectionWrapper(pp);
+        out << getParamTypeWithModeWrapper(pp);
     }
     out << ") -> " << getResultType();
 
@@ -659,8 +659,8 @@ Val* FuncType::_substituteImplOverride(ASTBuilder* astBuilder, SubstitutionSet s
     List<Type*> substParamTypes;
     for (Index pp = 0; pp < getParamCount(); pp++)
     {
-        auto substParamType = as<Type>(
-            getParamTypeWithDirectionWrapper(pp)->substituteImpl(astBuilder, subst, &diff));
+        auto substParamType =
+            as<Type>(getParamTypeWithModeWrapper(pp)->substituteImpl(astBuilder, subst, &diff));
         if (auto typePack = as<ConcreteTypePack>(substParamType))
         {
             // Unwrap the ConcreteTypePack and add each element as a parameter
@@ -695,7 +695,7 @@ Type* FuncType::_createCanonicalTypeOverride()
     List<Type*> canParamTypes;
     for (Index pp = 0; pp < getParamCount(); pp++)
     {
-        canParamTypes.add(getParamTypeWithDirectionWrapper(pp)->getCanonicalType());
+        canParamTypes.add(getParamTypeWithModeWrapper(pp)->getCanonicalType());
     }
 
     FuncType* canType = getCurrentASTBuilder()->getFuncType(
@@ -1406,14 +1406,9 @@ Val* TextureTypeBase::getFormat()
     return as<Type>(_getGenericTypeArg(this, 8));
 }
 
-Type* removeParamDirType(Type* type)
+bool isCopyableType(Type* type)
 {
-    for (auto paramDirType = as<ParamPassingModeType>(type); paramDirType;)
-    {
-        type = paramDirType->getValueType();
-        paramDirType = as<ParamPassingModeType>(type);
-    }
-    return type;
+    return !isNonCopyableType(type);
 }
 
 bool isNonCopyableType(Type* type)

source/slang/slang-ast-type.h

@@ -814,6 +814,23 @@ class NamedExpressionType : public Type
     NamedExpressionType(DeclRef<TypeDefDecl> inDeclRef) { setOperands(inDeclRef); }
 };
 
+/// Adjust a parameter-passing mode to account for the type of a parameter.
+///
+/// The `originalMode` should be the mode that would be used by default;
+/// usually this is a mode returned by `getExplicitlyDeclaredParamPassingMode()`
+/// or something similar.
+///
+/// The `paramType` should be the declared type of the parameter, not including
+/// any of the wrapper types that are used to represent parameter-passing modes.
+///
+/// This function is primarily concerned with adjusting a parameter-passing
+/// mode to account for non-copyable types, which may need different defaults
+/// than a copyable type.
+///
+ParamPassingMode adjustParamPassingModeBasedOnParamType(
+    ParamPassingMode originalMode,
+    Type* paramType);
+
 // A function type is defined by its parameter types
 // and its result type.
 FIDDLE()
@@ -851,7 +868,7 @@ class FuncType : public Type
     /// the possibility of encountering these wrappers, and handle
     /// them accordingly.
     ///
-    Type* getParamTypeWithDirectionWrapper(Index index) { return as<Type>(getOperand(index)); }
+    Type* getParamTypeWithModeWrapper(Index index) { return as<Type>(getOperand(index)); }
 
     /// Get the type of one of the function's parameters, by index.
     ///
@@ -872,14 +889,14 @@ class FuncType : public Type
 
     /// Get the parameter-passing mode of one of the function's parameters, by index.
     ///
-    ParamPassingMode getParamDirection(Index index);
+    ParamPassingMode getParamPassingMode(Index index);
 
     /// Combined information on the type and parameter-passing mode of a parameter.
     ///
     struct ParamInfo
     {
-        /// The parameter-passing mode used for the parameter.
-        ParamPassingMode direction = ParamPassingMode::In;
+        /// The parameter-passing mode for the parameter.
+        ParamPassingMode mode = ParamPassingMode::In;
 
         /// The user-perceived type of the parameter.
         Type* type = nullptr;
@@ -890,7 +907,7 @@ class FuncType : public Type
     ParamInfo getParamInfo(Index index)
     {
         ParamInfo info;
-        info.direction = getParamDirection(index);
+        info.mode = getParamPassingMode(index);
         info.type = getParamValueType(index);
         return info;
     }
@@ -1162,7 +1179,7 @@ class ModifiedType : public Type
     Val* _substituteImplOverride(ASTBuilder* astBuilder, SubstitutionSet subst, int* ioDiff);
 };
 
-Type* removeParamDirType(Type* type);
+bool isCopyableType(Type* type);
 bool isNonCopyableType(Type* type);
 
 } // namespace Slang

source/slang/slang-check-constraint.cpp

@@ -1110,8 +1110,8 @@ bool SemanticsVisitor::TryUnifyTypesByStructuralMatch(
                 if (!TryUnifyTypes(
                         constraints,
                         unifyCtx,
-                        fstFunType->getParamTypeWithDirectionWrapper(i),
-                        sndFunType->getParamTypeWithDirectionWrapper(i)))
+                        fstFunType->getParamTypeWithModeWrapper(i),
+                        sndFunType->getParamTypeWithModeWrapper(i)))
                     return false;
             }
             return TryUnifyTypes(

source/slang/slang-check-conversion.cpp

@@ -2103,8 +2103,8 @@ bool SemanticsVisitor::tryCoerceLambdaToFuncType(
     Index paramId = 0;
     for (auto param : invokeFunc->getParameters())
     {
-        auto paramType = getParamTypeWithDirectionWrapper(m_astBuilder, param);
-        auto toParamType = toFuncType->getParamTypeWithDirectionWrapper(paramId);
+        auto paramType = getParamTypeWithModeWrapper(m_astBuilder, param);
+        auto toParamType = toFuncType->getParamTypeWithModeWrapper(paramId);
         if (!paramType->equals(toParamType))
         {
             return false;

source/slang/slang-check-decl.cpp

@@ -4209,8 +4209,8 @@ bool SemanticsVisitor::doesSignatureMatchRequirement(
     {
         auto requiredParam = requiredParams[paramIndex];
         auto satisfyingParam = satisfyingParams[paramIndex];
-        if (getParameterDirection(requiredParam.getDecl()) !=
-            getParameterDirection(satisfyingParam.getDecl()))
+        if (getParamPassingMode(requiredParam.getDecl()) !=
+            getParamPassingMode(satisfyingParam.getDecl()))
             return false;
         auto requiredParamType = getType(m_astBuilder, requiredParam);
         auto satisfyingParamType = getType(m_astBuilder, satisfyingParam);
@@ -5697,16 +5697,16 @@ bool SemanticsVisitor::trySynthesizeMethodRequirementWitness(
                     auto synParam = *synParamIter;
                     auto calleeParam = *calleeParamIter;
                     if (!matchParamDirection(
-                            getParameterDirection(calleeParam),
-                            getParameterDirection(synParam)))
+                            getParamPassingMode(calleeParam),
+                            getParamPassingMode(synParam)))
                     {
                         if (outFailureDetails)
                         {
                             outFailureDetails->reason =
                                 WitnessSynthesisFailureReason::ParameterDirMismatch;
                             outFailureDetails->candidateMethod = declRefExpr->declRef;
-                            outFailureDetails->actualDir = getParameterDirection(calleeParam);
-                            outFailureDetails->expectedDir = getParameterDirection(synParam);
+                            outFailureDetails->actualDir = getParamPassingMode(calleeParam);
+                            outFailureDetails->expectedDir = getParamPassingMode(synParam);
                             outFailureDetails->paramDecl = calleeParam;
                         }
                         return false;
@@ -9553,50 +9553,7 @@ void SemanticsDeclHeaderVisitor::visitParamDecl(ParamDecl* paramDecl)
         checkMeshOutputDecl(paramDecl);
     }
 
-    if (auto declRefType = as<DeclRefType>(paramDecl->type.type))
-    {
-        if (declRefType->getDeclRef().getDecl()->findModifier<NonCopyableTypeAttribute>())
-        {
-            // Always pass a non-copyable type by reference.
-            // Remove all existing direction modifiers, and replace them with a single Ref modifier.
-            List<Modifier*> newModifiers;
-            bool hasRefModifier = false;
-            bool isMutable = false;
-            for (auto modifier : paramDecl->modifiers)
-            {
-                if (as<InModifier>(modifier))
-                {
-                    continue;
-                }
-                else if (as<InOutModifier>(modifier) || as<OutModifier>(modifier))
-                {
-                    isMutable = true;
-                    continue;
-                }
-                if (as<RefModifier>(modifier) || as<BorrowModifier>(modifier))
-                {
-                    hasRefModifier = true;
-                }
-                newModifiers.add(modifier);
-            }
-            if (!hasRefModifier)
-            {
-                if (isMutable)
-                    newModifiers.add(this->getASTBuilder()->create<RefModifier>());
-                else
-                    newModifiers.add(this->getASTBuilder()->create<BorrowModifier>());
-            }
-            paramDecl->modifiers.first = newModifiers.getFirst();
-            for (Index i = 0; i < newModifiers.getCount(); i++)
-            {
-                if (i < newModifiers.getCount() - 1)
-                    newModifiers[i]->next = newModifiers[i + 1];
-                else
-                    newModifiers[i]->next = nullptr;
-            }
-        }
-    }
-    else if (isTypePack(paramDecl->type.type))
+    if (isTypePack(paramDecl->type.type))
     {
         // For now, we only allow parameter packs to be `const`.
         bool hasConstModifier = false;
@@ -9605,13 +9562,26 @@ void SemanticsDeclHeaderVisitor::visitParamDecl(ParamDecl* paramDecl)
             if (as<OutModifier>(modifier) || as<InOutModifier>(modifier) ||
                 as<RefModifier>(modifier) || as<BorrowModifier>(modifier))
             {
+                // TODO(tfoley): The diagnostic in this case should probably not refer
+                // to the `const` modifier at all (since that is not actually what is
+                // required), and should instead note that a parameter pack may only
+                // be declared as a pure input parameter to a function (`in` or
+                // `borrow`).
+                //
                 getSink()->diagnose(modifier, Diagnostics::parameterPackMustBeConst);
             }
             else if (as<ConstModifier>(modifier))
             {
                 hasConstModifier = true;
             }
         }
+
+        // TODO(tfoley): Rather than actually changing the modifiers
+        // on the parameter itself, this kind of logic should probably
+        // be folded into whatever logic computes the `QualType` for a
+        // parameter, and ensure that parameter packs are never treated
+        // as l-values.
+        //
         if (!hasConstModifier)
         {
             auto constModifier = this->getASTBuilder()->create<ConstModifier>();
@@ -10093,7 +10063,7 @@ void SemanticsDeclHeaderVisitor::setFuncTypeIntoRequirementDecl(
     {
         auto paramInfo = funcType->getParamInfo(i);
         auto paramType = paramInfo.type;
-        auto paramDir = paramInfo.direction;
+        auto paramDir = paramInfo.mode;
 
         auto param = m_astBuilder->create<ParamDecl>();
         param->type.type = paramType;
@@ -12720,14 +12690,14 @@ void checkDerivativeAttributeImpl(
                 //
                 if (resolvedInvoke->arguments[ii]->type.type->equals(
                         ctx.getASTBuilder()->getErrorType()) ||
-                    funcType->getParamDirection(ii) != paramDirections[ii])
+                    funcType->getParamPassingMode(ii) != paramDirections[ii])
                 {
                     visitor->getSink()->diagnose(
                         attr,
                         Diagnostics::customDerivativeSignatureMismatchAtPosition,
                         ii,
                         qualTypeToString(argList[ii]->type),
-                        funcType->getParamTypeWithDirectionWrapper(ii)->toString());
+                        funcType->getParamTypeWithModeWrapper(ii)->toString());
                 }
             }
             // The `imaginaryArguments` list does not include the `this` parameter.
@@ -12882,7 +12852,7 @@ ArgsWithDirectionInfo getImaginaryArgsToFunc(
         arg->type.type = param->getType();
         arg->loc = loc;
         imaginaryArguments.add(arg);
-        directions.add(getParameterDirection(param));
+        directions.add(getParamPassingMode(param));
     }
     return {imaginaryArguments, directions, nullptr, ParamPassingMode::In};
 }
@@ -12938,7 +12908,7 @@ ArgsWithDirectionInfo getImaginaryArgsToForwardDerivative(
     List<ParamPassingMode> expectedParamDirections;
     for (auto param : originalFuncDecl->getParameters())
     {
-        expectedParamDirections.add(getParameterDirection(param));
+        expectedParamDirections.add(getParamPassingMode(param));
     }
 
     return {imaginaryArguments, expectedParamDirections, thisArgExpr, thisTypeDirection};
@@ -12995,7 +12965,7 @@ ArgsWithDirectionInfo getImaginaryArgsToBackwardDerivative(
         arg->type.type = param->getType();
         arg->loc = loc;
 
-        ParamPassingMode direction = getParameterDirection(param);
+        ParamPassingMode direction = getParamPassingMode(param);
 
         bool isDiffParam = (!param->findModifier<NoDiffModifier>());
         if (isDiffParam)

source/slang/slang-check-expr.cpp

@@ -2970,7 +2970,7 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr)
             Index paramCount = funcType->getParamCount();
             for (Index pp = 0; pp < paramCount; ++pp)
             {
-                auto paramType = funcType->getParamTypeWithDirectionWrapper(pp);
+                auto paramType = funcType->getParamTypeWithModeWrapper(pp);
                 Expr* argExpr = nullptr;
                 ParamDecl* paramDecl = nullptr;
                 if (pp < invoke->arguments.getCount())
@@ -3727,11 +3727,10 @@ Type* SemanticsVisitor::getForwardDiffFuncType(FuncType* originalType)
     for (Index i = 0; i < originalType->getParamCount(); i++)
     {
         if (auto jvpParamType =
-                _toDifferentialParamType(originalType->getParamTypeWithDirectionWrapper(i)))
+                _toDifferentialParamType(originalType->getParamTypeWithModeWrapper(i)))
             paramTypes.add(jvpParamType);
     }
-    FuncType* jvpType =
-        m_astBuilder->getOrCreate<FuncType>(paramTypes.getArrayView(), resultType, errorType);
+    FuncType* jvpType = m_astBuilder->getFuncType(paramTypes.getArrayView(), resultType, errorType);
 
     return jvpType;
 }
@@ -3753,7 +3752,7 @@ Type* SemanticsVisitor::getBackwardDiffFuncType(FuncType* originalType)
 
     for (Index i = 0; i < originalType->getParamCount(); i++)
     {
-        auto originalParamType = originalType->getParamTypeWithDirectionWrapper(i);
+        auto originalParamType = originalType->getParamTypeWithModeWrapper(i);
 
         if (auto outType = as<OutType>(originalParamType))
         {
@@ -3792,7 +3791,7 @@ Type* SemanticsVisitor::getBackwardDiffFuncType(FuncType* originalType)
     if (dOutType)
         paramTypes.add(dOutType);
 
-    return m_astBuilder->getOrCreate<FuncType>(paramTypes.getArrayView(), resultType, errorType);
+    return m_astBuilder->getFuncType(paramTypes.getArrayView(), resultType, errorType);
 }
 
 struct HigherOrderInvokeExprCheckingActions
@@ -4837,7 +4836,7 @@ Expr* SemanticsExprVisitor::visitLambdaExpr(LambdaExpr* lambdaExpr)
         genApp->arguments.add(returnTypeExp);
         for (auto param : getMembersOfType<ParamDecl>(m_astBuilder, lambdaExpr->paramScopeDecl))
         {
-            auto paramType = getParamTypeWithDirectionWrapper(m_astBuilder, param);
+            auto paramType = getParamTypeWithModeWrapper(m_astBuilder, param);
             auto paramTypeExp = synthesizer.emitStaticTypeExpr(paramType);
             genApp->arguments.add(paramTypeExp);
         }