From 0257cb001b6f5c31e4ac0435d546fe638a17c48a Mon Sep 17 00:00:00 2001 From: Yong He Date: Thu, 16 Oct 2025 11:52:38 -0700 Subject: Fix wrong diagnostic when checking identical casting expr. (#8727) `SemanticsVisitor::CheckInvokeExprWithCheckedOperands` made several references to `expr` parameter in its `inout` parameter l-value-ness validation logic to access arguments, which is wrong because `expr` is not necessarily the same as `result`/`invoke` (the result of calling `ResolveInvoke()` in the first line of the function. Changing it to `invoke` for consistency. Also add a special case logic to return early in case the resolved invoke expr is `argument[0]` when the original invoke expr is `T(funcThatReturnsT())`. Closes #8659. --- source/slang/slang-check-expr.cpp | 54 ++++++++++++++++++++++++--------------- tests/bugs/gh-8659.slang | 16 ++++++++++++ 2 files changed, 49 insertions(+), 21 deletions(-) create mode 100644 tests/bugs/gh-8659.slang diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp index b8e68e28b..7935ba96e 100644 --- a/source/slang/slang-check-expr.cpp +++ b/source/slang/slang-check-expr.cpp @@ -2941,8 +2941,16 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr) if (!invoke->functionExpr) return rs; - // if this is still an invoke expression, test arguments passed to inout/out parameter are - // LValues + // if this is still an invoke expression, test arguments passed to inout/out parameter + // are LValues. + // + // A special case that allows us to skip this validation is when `expr` is an identical type + // cast, e.g. `(T)(funcThatReturnsT())`. In this case `ResolveInvoke(expr)` will simply + // return the argument expr `funcThatReturnsT()`. And we can skip rerunning any `out` param + // validation logic on the inner expr. + if (expr->arguments.getCount() == 1 && invoke == expr->arguments[0]) + return rs; + if (auto funcType = as(invoke->functionExpr->type)) { if (!funcType->getErrorType()->equals(m_astBuilder->getBottomType())) @@ -2965,9 +2973,9 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr) auto paramType = funcType->getParamTypeWithDirectionWrapper(pp); Expr* argExpr = nullptr; ParamDecl* paramDecl = nullptr; - if (pp < expr->arguments.getCount()) + if (pp < invoke->arguments.getCount()) { - argExpr = expr->arguments[pp]; + argExpr = invoke->arguments[pp]; if (funcDeclBase) paramDecl = funcDeclBase->getParameters()[pp]; } @@ -2989,18 +2997,19 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr) auto implicitCastExpr = as(argExpr); // NOTE: - // This is currently only enabled for in/inout based scenarios. Ie NOT - // ref. + // This is currently only enabled for in/inout based scenarios. Ie + // NOT ref. // // Depending on the target there can be an issue around atomics. // The fall back transformation with InOut/OutImplicitCast is to // introduce a temporary, and do the work on that and copy back. // - // This doesn't work with an atomic. So the work around is to not enable - // the transformation with ref types, which atomics are defined on. + // This doesn't work with an atomic. So the work around is to not + // enable the transformation with ref types, which atomics are + // defined on. // - // An argument can be made that transformation shouldn't apply to the - // ref scenario in general. + // An argument can be made that transformation shouldn't apply to + // the ref scenario in general. if (implicitCastExpr && as(paramType) && _canLValueCoerce( implicitCastExpr->arguments[0]->type, @@ -3014,10 +3023,11 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr) // a += b; // ``` // That strictly speaking it's not allowed, but we are going to - // allow it for now for situations were the types are uint/int and - // vector/matrix varieties of those types + // allow it for now for situations were the types are uint/int + // and vector/matrix varieties of those types // - // Then in lowering we are going to insert code to do something like + // Then in lowering we are going to insert code to do something + // like // ``` // var OutType: tmp = arg; // f(... tmp); @@ -3042,13 +3052,14 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr) *implicitCastExpr); } - // Replace the expression. This should make this situation easier to - // detect. - expr->arguments[pp] = lValueImplicitCast; + // Replace the expression. This should make this situation + // easier to detect. + invoke->arguments[pp] = lValueImplicitCast; } else if (!as(argExpr->type)) { - // Emit additional diagnostic for invalid pointer taking operations + // Emit additional diagnostic for invalid pointer taking + // operations auto funcDeclRef = funcDeclRefExpr ? getDeclRef(m_astBuilder, funcDeclRefExpr) : DeclRef(); @@ -3086,16 +3097,17 @@ Expr* SemanticsVisitor::CheckInvokeExprWithCheckedOperands(InvokeExpr* expr) // Try and determine reason for failure if (as(paramType)) { - // Ref types are not allowed to use this mechanism because - // it breaks atomics + // Ref types are not allowed to use this mechanism + // because it breaks atomics diagnostic = &Diagnostics::implicitCastUsedAsLValueRef; } else if (!_canLValueCoerce( implicitCastExpr->arguments[0]->type, implicitCastExpr->type)) { - // We restict what types can use this mechanism - currently - // int/uint and same sized matrix/vectors of those types. + // We restict what types can use this mechanism - + // currently int/uint and same sized matrix/vectors of + // those types. diagnostic = &Diagnostics::implicitCastUsedAsLValueType; } else diff --git a/tests/bugs/gh-8659.slang b/tests/bugs/gh-8659.slang new file mode 100644 index 000000000..599c09b3b --- /dev/null +++ b/tests/bugs/gh-8659.slang @@ -0,0 +1,16 @@ +//TEST:COMPARE_COMPUTE(filecheck-buffer=CHECK):-vk -output-using-type -emit-spirv-directly + +//TEST_INPUT: set ptr = ubuffer(data=[0 0 0 0], stride=4) +uniform uint* ptr; + +//TEST_INPUT: set outputBuffer = out ubuffer(data=[0 0 0 0], stride=4) +RWStructuredBuffer outputBuffer; + +[numthreads(1,1,1)] +void computeMain() +{ + let indices = (uint*)&ptr[0]; + *indices = 100; + // CHECK: 100 + outputBuffer[0] = ptr[0]; +} \ No newline at end of file -- cgit v1.2.3