From a1a30bb5790ef0e6ec278d6e0b52501debd2d018 Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Tue, 26 Apr 2022 15:41:35 -0700 Subject: [PATCH 01/10] Disallow illegal Atomic targets + propagate consts This disallows a number of illegal destination parameters to atomic operations. This includes SRVs and other const values, non-groupshared and non-resourse variables, members of typed resources, and bitfield members of any resource. In addition, this correctly propagates const information so that they can be properly rejected and also the information is reflected. This involves the consolidation and earlier detection of a number of these failures as well as an expansion of that detection. Also adds validation checks that the targets are not const and either UAVs or groupshared address space. Add compilation tests as well as validation tests for all. Fixes #4319 Fixes #4377 Fixes #4437 --- docs/DXIL.rst | 3 + lib/HLSL/DxilValidation.cpp | 16 ++- lib/HLSL/HLOperationLower.cpp | 122 +++++++--------- tools/clang/lib/Sema/SemaExpr.cpp | 2 +- tools/clang/lib/Sema/SemaHLSL.cpp | 11 +- tools/clang/test/DXILValidation/atomics.hlsl | 91 ++++++++++++ .../errors/write_const_arrays.hlsl | 61 ++++++++ .../atomic/atomic_on_bitfields.hlsl | 36 +++++ .../intrinsics/atomic/atomic_on_members.hlsl | 22 +++ .../intrinsics/atomic/atomic_restypes.hlsl | 131 ++++++++++++++++++ .../operators/swizzle/swizzleAtomic2.hlsl | 17 ++- tools/clang/unittests/HLSL/ValidationTest.cpp | 106 ++++++++++++++ utils/hct/hctdb.py | 3 + 13 files changed, 542 insertions(+), 79 deletions(-) create mode 100644 tools/clang/test/DXILValidation/atomics.hlsl create mode 100644 tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/write_const_arrays.hlsl create mode 100644 tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_bitfields.hlsl create mode 100644 tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_members.hlsl create mode 100644 tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl diff --git a/docs/DXIL.rst b/docs/DXIL.rst index d3f14c70f7..97df3f1b5e 100644 --- a/docs/DXIL.rst +++ b/docs/DXIL.rst @@ -2997,6 +2997,9 @@ FLOW.FUNCTIONCALL Function with parameter is not permitt FLOW.NORECUSION Recursion is not permitted. FLOW.REDUCIBLE Execution flow must be reducible. INSTR.ALLOWED Instructions must be of an allowed type. +INSTR.ATOMICCONST Constant destination to atomic. +INSTR.ATOMICINTRINNONUAV Non-UAV destination to atomic intrinsic. +INSTR.ATOMICOPNONGROUPSHARED Non-groupshared destination to atomic operation. INSTR.ATTRIBUTEATVERTEXNOINTERPOLATION Attribute %0 must have nointerpolation mode in order to use GetAttributeAtVertex function. INSTR.BARRIERMODEFORNONCS sync in a non-Compute/Amplification/Mesh Shader must only sync UAV (sync_uglobal). INSTR.BARRIERMODENOMEMORY sync must include some form of memory barrier - _u (UAV) and/or _g (Thread Group Shared Memory). Only _t (thread group sync) is optional. diff --git a/lib/HLSL/DxilValidation.cpp b/lib/HLSL/DxilValidation.cpp index c094a5c3a8..0d19ceb5ad 100644 --- a/lib/HLSL/DxilValidation.cpp +++ b/lib/HLSL/DxilValidation.cpp @@ -2192,6 +2192,10 @@ static void ValidateDxilOperationCallInProfile(CallInst *CI, if ((pOverloadType->isIntegerTy(64)) && !pSM->IsSM66Plus()) ValCtx.EmitInstrFormatError(CI, ValidationRule::SmOpcodeInInvalidFunction, {"64-bit atomic operations", "Shader Model 6.6+"}); + Value *Handle = CI->getOperand(DXIL::OperandIndex::kAtomicBinOpHandleOpIdx); + if (!isa(Handle) || + ValCtx.GetResourceFromVal(Handle).getResourceClass() != DXIL::ResourceClass::UAV) + ValCtx.EmitInstrError(CI, ValidationRule::InstrAtomicIntrinNonUAV); } break; case DXIL::OpCode::CreateHandle: if (ValCtx.isLibProfile) { @@ -3180,11 +3184,19 @@ static void ValidateFunctionBody(Function *F, ValidationContext &ValCtx) { } break; case Instruction::AtomicCmpXchg: case Instruction::AtomicRMW: { - Type *T = cast(I.getOperand(AtomicRMWInst::getPointerOperandIndex())->getType())->getElementType(); + Value *Ptr = I.getOperand(AtomicRMWInst::getPointerOperandIndex()); + PointerType *ptrType = cast(Ptr->getType()); + Type *elType = ptrType->getElementType(); const ShaderModel *pSM = ValCtx.DxilMod.GetShaderModel(); - if ((T->isIntegerTy(64)) && !pSM->IsSM66Plus()) + if ((elType->isIntegerTy(64)) && !pSM->IsSM66Plus()) ValCtx.EmitInstrFormatError(&I, ValidationRule::SmOpcodeInInvalidFunction, {"64-bit atomic operations", "Shader Model 6.6+"}); + if (ptrType->getAddressSpace() != DXIL::kTGSMAddrSpace) { + ValCtx.EmitInstrError(&I, ValidationRule::InstrAtomicOpNonGroupshared); + } else if (GlobalVariable *GV = dyn_cast(Ptr)) { + if(GV->isConstant()) + ValCtx.EmitInstrError(&I, ValidationRule::InstrAtomicConst); + } } break; } diff --git a/lib/HLSL/HLOperationLower.cpp b/lib/HLSL/HLOperationLower.cpp index 3b49e7af7d..7c9ead79e5 100644 --- a/lib/HLSL/HLOperationLower.cpp +++ b/lib/HLSL/HLOperationLower.cpp @@ -4421,6 +4421,40 @@ static Value* SkipAddrSpaceCast(Value* Ptr) { return Ptr; } +// For known non-groupshared, verify that the destination param is valid +void ValidateAtomicDestination(CallInst *CI, HLObjectOperationLowerHelper *pObjHelper) { + Value *dest = CI->getArgOperand(HLOperandIndex::kInterlockedDestOpIndex); + GetElementPtrInst *gep = nullptr; + + // If we encounter a gep, we may provide a more specific error message + while (isa(dest)) { + gep = cast(dest); + dest = gep->getPointerOperand(); + } + + // Confirm that dest is a UAV + if (CallInst *destCall = dyn_cast(dest)) { + hlsl::HLOpcodeGroup group = hlsl::GetHLOpcodeGroup(destCall->getCalledFunction()); + while (group == HLOpcodeGroup::HLSubscript) { + dest = destCall->getArgOperand(HLOperandIndex::kSubscriptObjectOpIdx); + if (!isa(dest)) + break; + destCall = cast(dest); + group = hlsl::GetHLOpcodeGroup(destCall->getCalledFunction()); + } + DXIL::ResourceKind RK = pObjHelper->GetRK(destCall); + if (DXIL::IsStructuredBuffer(RK)) + return; // no errors + if (DXIL::IsTyped(RK)) { + if (gep) + dxilutil::EmitErrorOnInstruction(CI, "Typed resources used in atomic operations must have a scalar element type."); + return; // error emitted or else no errors + } + } + + dxilutil::EmitErrorOnInstruction(CI, "Atomic operation targets must be groupshared or UAV."); +} + Value *TranslateIopAtomicBinaryOperation(CallInst *CI, IntrinsicOp IOP, DXIL::OpCode opcode, HLOperationLowerHelper &helper, HLObjectOperationLowerHelper *pObjHelper, bool &Translated) { @@ -4431,11 +4465,13 @@ Value *TranslateIopAtomicBinaryOperation(CallInst *CI, IntrinsicOp IOP, if (addressSpace == DXIL::kTGSMAddrSpace) TranslateSharedMemAtomicBinOp(CI, IOP, addr); else { - // buffer atomic translated in TranslateSubscript. - // Do nothing here. - // Mark not translated. + // If not groupshared, we either have an error case or will translate + // the atomic op in the process of translating users of the subscript operator + // Mark not translated and validate dest param Translated = false; + ValidateAtomicDestination(CI, pObjHelper); } + return nullptr; } @@ -4480,10 +4516,11 @@ Value *TranslateIopAtomicCmpXChg(CallInst *CI, IntrinsicOp IOP, if (addressSpace == DXIL::kTGSMAddrSpace) TranslateSharedMemAtomicCmpXChg(CI, addr); else { - // buffer atomic translated in TranslateSubscript. - // Do nothing here. - // Mark not translated. + // If not groupshared, we either have an error case or will translate + // the atomic op in the process of translating users of the subscript operator + // Mark not translated and validate dest param Translated = false; + ValidateAtomicDestination(CI, pObjHelper); } return nullptr; @@ -7698,50 +7735,10 @@ void TranslateDefaultSubscript(CallInst *CI, HLOperationLowerHelper &helper, HL LI->eraseFromParent(); continue; } - if (!isa(GEPUser)) { - // Invalid operations. - Translated = false; - dxilutil::EmitErrorOnInstruction(GEP, "Invalid operation on typed buffer."); - return; - } - CallInst *userCall = cast(GEPUser); - HLOpcodeGroup group = - hlsl::GetHLOpcodeGroupByName(userCall->getCalledFunction()); - if (group != HLOpcodeGroup::HLIntrinsic) { - // Invalid operations. - Translated = false; - dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer."); - return; - } - unsigned opcode = hlsl::GetHLOpcode(userCall); - IntrinsicOp IOP = static_cast(opcode); - switch (IOP) { - case IntrinsicOp::IOP_InterlockedAdd: - case IntrinsicOp::IOP_InterlockedAnd: - case IntrinsicOp::IOP_InterlockedExchange: - case IntrinsicOp::IOP_InterlockedMax: - case IntrinsicOp::IOP_InterlockedMin: - case IntrinsicOp::IOP_InterlockedUMax: - case IntrinsicOp::IOP_InterlockedUMin: - case IntrinsicOp::IOP_InterlockedOr: - case IntrinsicOp::IOP_InterlockedXor: - case IntrinsicOp::IOP_InterlockedCompareStore: - case IntrinsicOp::IOP_InterlockedCompareExchange: - case IntrinsicOp::IOP_InterlockedCompareStoreFloatBitwise: - case IntrinsicOp::IOP_InterlockedCompareExchangeFloatBitwise: { - // Invalid operations. - Translated = false; - dxilutil::EmitErrorOnInstruction( - userCall, "Typed resources used in atomic operations must have a scalar element type."); - return; - } break; - default: - // Invalid operations. - Translated = false; - dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer."); - return; - break; - } + // Invalid operations. + Translated = false; + dxilutil::EmitErrorOnInstruction(GEP, "Invalid operation on typed buffer."); + return; } GEP->eraseFromParent(); } else { @@ -7754,29 +7751,8 @@ void TranslateDefaultSubscript(CallInst *CI, HLOperationLowerHelper &helper, HL if (RC == DXIL::ResourceClass::SRV) { // Invalid operations. Translated = false; - switch (IOP) { - case IntrinsicOp::IOP_InterlockedAdd: - case IntrinsicOp::IOP_InterlockedAnd: - case IntrinsicOp::IOP_InterlockedExchange: - case IntrinsicOp::IOP_InterlockedMax: - case IntrinsicOp::IOP_InterlockedMin: - case IntrinsicOp::IOP_InterlockedUMax: - case IntrinsicOp::IOP_InterlockedUMin: - case IntrinsicOp::IOP_InterlockedOr: - case IntrinsicOp::IOP_InterlockedXor: - case IntrinsicOp::IOP_InterlockedCompareStore: - case IntrinsicOp::IOP_InterlockedCompareExchange: - case IntrinsicOp::IOP_InterlockedCompareStoreFloatBitwise: - case IntrinsicOp::IOP_InterlockedCompareExchangeFloatBitwise: { - dxilutil::EmitErrorOnInstruction( - userCall, "Atomic operation targets must be groupshared or UAV."); - return; - } break; - default: - dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer."); - return; - break; - } + dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer."); + return; } switch (IOP) { case IntrinsicOp::IOP_InterlockedAdd: { diff --git a/tools/clang/lib/Sema/SemaExpr.cpp b/tools/clang/lib/Sema/SemaExpr.cpp index f18349241a..cc7a975058 100644 --- a/tools/clang/lib/Sema/SemaExpr.cpp +++ b/tools/clang/lib/Sema/SemaExpr.cpp @@ -4285,7 +4285,7 @@ Sema::CreateBuiltinArraySubscriptExpr(Expr *Base, SourceLocation LLoc, // We need to make sure to preserve qualifiers on array types, since these // are in effect references. if (LHSTy.hasQualifiers()) - ResultType.setLocalFastQualifiers(LHSTy.getLocalFastQualifiers()); + ResultType.setLocalFastQualifiers(LHSTy.getQualifiers().getFastQualifiers()); } else { // HLSL Change Ends Diag(LHSExp->getLocStart(), diag::ext_subscript_non_lvalue) << diff --git a/tools/clang/lib/Sema/SemaHLSL.cpp b/tools/clang/lib/Sema/SemaHLSL.cpp index e9abedfe68..8659290c38 100644 --- a/tools/clang/lib/Sema/SemaHLSL.cpp +++ b/tools/clang/lib/Sema/SemaHLSL.cpp @@ -6155,14 +6155,23 @@ bool HLSLExternalSource::MatchArguments( } } + ASTContext &actx = m_sema->getASTContext(); // Usage if (pIntrinsicArg->qwUsage & AR_QUAL_OUT) { - if (pCallArg->getType().isConstQualified()) { + if (pType.isConstant(actx)) { // Can't use a const type in an out or inout parameter. badArgIdx = std::min(badArgIdx, iArg); } } + // Catch invalid atomic dest parameters + if (iArg == kAtomicDstOperandIdx && + IsAtomicOperation(static_cast(pIntrinsic->Op))) { + // bitfield error is confusing + if (pType.isConstant(actx) || pCallArg->getObjectKind() == OK_BitField) { + badArgIdx = std::min(badArgIdx, iArg); + } + } iArg++; } diff --git a/tools/clang/test/DXILValidation/atomics.hlsl b/tools/clang/test/DXILValidation/atomics.hlsl new file mode 100644 index 0000000000..9a034f92ee --- /dev/null +++ b/tools/clang/test/DXILValidation/atomics.hlsl @@ -0,0 +1,91 @@ +// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=gs_var -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=gs_arr -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=cb_var -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=cb_arr -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=sc_var -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=sc_arr -DIDX=[0] %s | %FileCheck %s +// These two are different because they aren't const, so are caught later +// RUN: %dxc -T cs_6_0 -DDEST=loc_var -DIDX= %s | %FileCheck %s -check-prefix=CHKLOC +// RUN: %dxc -T cs_6_0 -DDEST=loc_arr -DIDX=[0] %s | %FileCheck %s -check-prefix=CHKLOC + + +// Test various Interlocked ops using different invalid destination memory types +// The way the dest param of atomic ops is lowered is unique and missed a lot of +// these invalid uses. There are a few points where the lowering branches depending +// on the memory type, so this tries to cover all those branches: +// groupshared, cbuffers, structbuffers, other resources, and other non-resources + +// Valid resources for atomics to create valid output that will later be manipulated to test the validator +RWStructuredBuffer rw_structbuf; +RWBuffer rw_buf; +RWTexture1D rw_tex; + +StructuredBuffer ro_structbuf; +Buffer ro_buf; +Texture1D ro_tex; + +RWBuffer rw_bufswiz; +RWTexture1D rw_texswiz; + +struct TexCoords { + uint s, t, r, q; +}; + +RWBuffer rw_bufmemb; + +struct TexCoordsBits { + uint s : 8; + uint t : 8; + uint r : 8; + uint q : 8; +}; + +RWBuffer rw_bufbits; +RWStructuredBuffer rw_strbits; + +const groupshared uint cgs_var; + +groupshared uint gs_var; +groupshared TexCoordsBits gs_bits; + +RWStructuredBuffer output; // just something to keep the variables alive + +cbuffer CB { + uint cb_var; +} +uint cb_gvar; + +#if __SHADER_TARGET_STAGE == __SHADER_STAGE_LIBRARY +void init(out uint i); // To force an alloca pointer to use with atomic op +#else +void init(out uint i) {i = 0;} +#endif + +[numthreads(1,1,1)] +void main(uint ix : SV_GroupIndex) { + + uint res; + init(res); + + InterlockedAdd(rw_structbuf[ix], 1); + InterlockedCompareStore(rw_structbuf[ix], 1, 2); + + InterlockedAdd(rw_buf[ix], 1); + InterlockedCompareStore(rw_buf[ix], 1, 2); + + InterlockedAdd(rw_tex[ix], 1); + InterlockedCompareStore(rw_tex[ix], 1, 2); + + InterlockedAdd(gs_var, 1); + InterlockedCompareStore(gs_var, 1, 2); + + // Token usages of the invalid resources and variables so they are available in the output + output[ix] = ix + cb_var + cb_gvar + rw_bufbits[ix].s + rw_strbits[ix].s + cgs_var + gs_bits.q + + rw_bufswiz[ix].x + rw_texswiz[ix].x + ro_structbuf[ix] + ro_buf[ix] + ro_tex[ix] + rw_bufmemb[ix].s; +} diff --git a/tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/write_const_arrays.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/write_const_arrays.hlsl new file mode 100644 index 0000000000..a445fe73af --- /dev/null +++ b/tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/write_const_arrays.hlsl @@ -0,0 +1,61 @@ +// RUN: %dxc -T cs_6_0 _HV 2021 %s | FileCheck %s + +// Array subscripts lost their qualifiers including const due to taking a different +// path for HLSL on account of having no array to ptr decay +// This test verifies that subscripts of constant and cbuf arrays can't be assigned +// or otherwise altered by a few mechanisms + +StructuredBuffer g_robuf; +Texture1D g_tex; +uint g_cbuf[4]; + +const groupshared uint gs_val[4]; + +[NumThreads(1, 1, 1)] +void main() { + const uint local[4]; + + //CHECK: error: read-only variable is not assignable + //CHECK: error: read-only variable is not assignable + //CHECK: error: read-only variable is not assignable + //CHECK: error: cannot assign to return value + // Assigning using assignment operator + local[0] = 0; + gs_val[0] = 0; + g_cbuf[0] = 0; + g_robuf[0] = 0; + + // Assigning using out param of builtin function + //CHECK: error: no matching function for call to 'sincos' + //CHECK: error: no matching function for call to 'sincos' + //CHECK: error: no matching function for call to 'sincos' + //CHECK: error: no matching function for call to 'sincos' + sincos(0.0, local[0], local[0]); + sincos(0.0, gs_val[0], gs_val[0]); + sincos(0.0, g_cbuf[0], g_cbuf[0]); + sincos(0.0, g_robuf[0], g_robuf[0]); + + + // Assigning using out param of method + //CHECK: error: no matching member function for call to 'GetDimensions' + //CHECK: error: no matching member function for call to 'GetDimensions' + //CHECK: error: no matching member function for call to 'GetDimensions' + //CHECK: error: no matching member function for call to 'GetDimensions' + g_tex.GetDimensions(local[0]); + g_tex.GetDimensions(gs_val[0]); + g_tex.GetDimensions(g_cbuf[0]); + g_tex.GetDimensions(g_robuf[0]); + + + // Assigning using dest param of atomics + // Distinct because of special handling of atomics dest param + //CHECK: error: no matching function for call to 'InterlockedAdd' + //CHECK: error: no matching function for call to 'InterlockedAdd' + //CHECK: error: no matching function for call to 'InterlockedAdd' + //CHECK: error: no matching function for call to 'InterlockedAdd' + InterlockedAdd(local[0], 1); + InterlockedAdd(gs_val[0], 1); + InterlockedAdd(g_cbuf[0], 1); + InterlockedAdd(g_robuf[0], 1); + +} diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_bitfields.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_bitfields.hlsl new file mode 100644 index 0000000000..643fce1511 --- /dev/null +++ b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_bitfields.hlsl @@ -0,0 +1,36 @@ +// RUN: %dxc -HV 2021 -T cs_6_0 %s | FileCheck %s + +// Ensure that atomic operations fail when used with bitfields +// Use structured and typed buffers as the resources that can use structs +// and also both binary op and exchange atomic ops as either difference +// can cause the compiler to take different paths + +struct TexCoords { + uint s : 8; + uint t : 8; + uint r : 8; + uint q : 8; +}; + +RWBuffer buf; +RWStructuredBuffer str; +groupshared TexCoords gs; + +[numthreads(8,8,1)] +void main( uint2 tid : SV_DispatchThreadID) { + + // CHECK: error: no matching function for call to 'InterlockedOr' + // CHECK: error: no matching function for call to 'InterlockedCompareStore' + InterlockedOr(buf[tid.y].q, 2); + InterlockedCompareStore(buf[tid.y].q, 3, 1); + + // CHECK: error: no matching function for call to 'InterlockedOr' + // CHECK: error: no matching function for call to 'InterlockedCompareStore' + InterlockedOr(str[tid.y].q, 2); + InterlockedCompareStore(str[tid.y].q, 3, 1); + + // CHECK: error: no matching function for call to 'InterlockedOr' + // CHECK: error: no matching function for call to 'InterlockedCompareStore' + InterlockedOr(gs.q, 2); + InterlockedCompareStore(gs.q, 3, 1); +} diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_members.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_members.hlsl new file mode 100644 index 0000000000..eab171ad9a --- /dev/null +++ b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_members.hlsl @@ -0,0 +1,22 @@ +// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedOr %s | FileCheck %s +// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedCompareStore %s | FileCheck %s + +// Ensure that atomic operations fail when used with struct members of a typed resource +// The only typed resource that can use structs is RWBuffer +// Use a binary op and an exchange op because they use different code paths + +struct TexCoords { + uint s, t, r, q; +}; + +RWBuffer bufA; + +[numthreads(8,8,1)] +void main( uint3 gtid : SV_GroupThreadID , uint gid : SV_GroupIndex) +{ + uint orig = 0; + TexCoords tc = {0, 0, 0, 0}; + + // CHECK: error: Typed resources used in atomic operations must have a scalar element type. + OP(bufA[gid].q, 2, orig); +} diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl new file mode 100644 index 0000000000..bb43b060fb --- /dev/null +++ b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl @@ -0,0 +1,131 @@ +// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=gs_var -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=gs_arr -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=cb_var -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=cb_arr -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=sc_var -DIDX= %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=sc_arr -DIDX=[0] %s | %FileCheck %s +// These are different because they aren't const, so are caught later +// RUN: %dxc -T cs_6_0 -DDEST=loc_var -DIDX= %s | %FileCheck %s -check-prefix=CHKLOC +// RUN: %dxc -T cs_6_0 -DDEST=loc_arr -DIDX=[0] %s | %FileCheck %s -check-prefix=CHKLOC +// RUN: %dxc -T cs_6_0 -DDEST=ix -DIDX= %s | %FileCheck %s -check-prefix=CHKLOC + + +// Test various Interlocked ops using different invalid destination memory types +// The way the dest param of atomic ops is lowered is unique and missed a lot of +// these invalid uses. There are a few points where the lowering branches depending +// on the memory type, so this tries to cover all those branches: +// groupshared, cbuffers, structbuffers, other resources, and other non-resources + +StructuredBuffer ro_structbuf; +Buffer ro_buf; +Texture1D ro_tex; + +const groupshared uint gs_var; +const groupshared uint gs_arr[4]; + +RWStructuredBuffer output; // just something to keep the variables live + +cbuffer CB { + uint cb_var; + uint cb_arr[4]; +} + +static const uint sc_var = 1; +static const uint sc_arr[4] = {0,1,2,3}; + +[numthreads(1,1,1)] +void main(uint ix : SV_GroupIndex) { + + uint loc_var; + uint loc_arr[4]; + + int val = 1; + uint comp = 1; + uint orig; + + // add + // CHECK: error: no matching function for call to 'InterlockedAdd' + // CHECK: note: candidate function not viable: no known conversion from + // CHECK: error: no matching function for call to 'InterlockedAdd' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedAdd(DEST IDX, val); + InterlockedAdd(DEST IDX, val, orig); + + // min + // CHECK: error: no matching function for call to 'InterlockedMin' + // CHECK: note: candidate function not viable: no known conversion from + // CHECK: error: no matching function for call to 'InterlockedMin' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedMin(DEST IDX, val); + InterlockedMin(DEST IDX, val, orig); + + // max + // CHECK: error: no matching function for call to 'InterlockedMax' + // CHECK: note: candidate function not viable: no known conversion from + // CHECK: error: no matching function for call to 'InterlockedMax' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedMax(DEST IDX, val); + InterlockedMax(DEST IDX, val, orig); + + // and + // CHECK: error: no matching function for call to 'InterlockedAnd' + // CHECK: note: candidate function not viable: no known conversion from + // CHECK: error: no matching function for call to 'InterlockedAnd' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedAnd(DEST IDX, val); + InterlockedAnd(DEST IDX, val, orig); + + // or + // CHECK: error: no matching function for call to 'InterlockedOr' + // CHECK: note: candidate function not viable: no known conversion from + // CHECK: error: no matching function for call to 'InterlockedOr' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedOr(DEST IDX, val); + InterlockedOr(DEST IDX, val, orig); + + // xor + // CHECK: error: no matching function for call to 'InterlockedXor' + // CHECK: note: candidate function not viable: no known conversion from + // CHECK: error: no matching function for call to 'InterlockedXor' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedXor(DEST IDX, val); + InterlockedXor(DEST IDX, val, orig); + + // compareStore + // CHECK: error: no matching function for call to 'InterlockedCompareStore' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedCompareStore(DEST IDX, comp, val); + + // exchange + // CHECK: error: no matching function for call to 'InterlockedExchange' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedExchange(DEST IDX, val, orig); + + // compareExchange + // CHECK: error: no matching function for call to 'InterlockedCompareExchange' + // CHECK: note: candidate function not viable: no known conversion from + // CHKLOC: error: Atomic operation targets must be groupshared or UAV + InterlockedCompareExchange(DEST IDX, comp, val, orig); + + output[ix] = (float)DEST IDX; +} diff --git a/tools/clang/test/HLSLFileCheck/hlsl/operators/swizzle/swizzleAtomic2.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/operators/swizzle/swizzleAtomic2.hlsl index 11df30ecc9..b2c94877aa 100644 --- a/tools/clang/test/HLSLFileCheck/hlsl/operators/swizzle/swizzleAtomic2.hlsl +++ b/tools/clang/test/HLSLFileCheck/hlsl/operators/swizzle/swizzleAtomic2.hlsl @@ -1,13 +1,26 @@ // RUN: %dxc -E main -T cs_6_0 %s | FileCheck %s -// CHECK: Typed resources used in atomic operations must have a scalar element type +// Check that attempting to use atomic ops on swizzled typed resource members will fail +// Use representatives of atomic binops and cmpexchange type atomic operations. +// Include both typed buffer and texture resources RWBuffer bufA; +RWTexture1D texA; [numthreads(8,8,1)] void main( uint2 tid : SV_DispatchThreadID, uint2 gid : SV_GroupID, uint2 gtid : SV_GroupThreadID, uint gidx : SV_GroupIndex ) { + // CHECK: Typed resources used in atomic operations must have a scalar element type + // CHECK: Typed resources used in atomic operations must have a scalar element type bufA[tid.x] = gid.x; bufA[tid.y].z = gid.y; InterlockedOr(bufA[tid.y].y, 2); - } + InterlockedCompareStore(bufA[tid.y].x, 3, 1); + + // CHECK: Typed resources used in atomic operations must have a scalar element type + // CHECK: Typed resources used in atomic operations must have a scalar element type + texA[tid.x] = gid.x; + texA[tid.y].z = gid.y; + InterlockedOr(texA[tid.y].y, 2); + InterlockedCompareStore(texA[tid.y].x, 3, 1); +} diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index e2d87005ad..7b009323fd 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -300,6 +300,9 @@ class ValidationTest : public ::testing::Test { TEST_METHOD(ValidateVersionNotAllowed) TEST_METHOD(CreateHandleNotAllowedSM66) + TEST_METHOD(AtomicsConsts) + TEST_METHOD(AtomicsInvalidDests) + dxc::DxcDllSupport m_dllSupport; VersionSupportInfo m_ver; @@ -3902,3 +3905,106 @@ TEST_F(ValidationTest, CreateHandleNotAllowedSM66) { "opcode 'CreateHandle' should only be used in 'non-library targets'", true); } + +// Check for validation errors for various const dests to atomics +TEST_F(ValidationTest, AtomicsConsts) { + if (m_ver.SkipDxilVersion(1, 7)) return; + std::vector pArguments = { L"-HV", L"2021" }; + + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_structbuf_UAV_structbuf"}, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %ro_structbuf_texture_structbuf"}, + "Non-UAV destination to atomic intrinsic.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_structbuf_UAV_structbuf"}, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %ro_structbuf_texture_structbuf"}, + "Non-UAV destination to atomic intrinsic.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_buf_UAV_buf"}, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %ro_buf_texture_buf"}, + "Non-UAV destination to atomic intrinsic.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_buf_UAV_buf"}, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %ro_buf_texture_buf"}, + "Non-UAV destination to atomic intrinsic.", + false); + + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_tex_UAV_1d"}, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %ro_tex_texture_1d"}, + "Non-UAV destination to atomic intrinsic.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_tex_UAV_1d"}, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %ro_tex_texture_1d"}, + "Non-UAV destination to atomic intrinsic.", + false); + + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_buf_UAV_buf"}, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %CB_cbuffer"}, + "Non-UAV destination to atomic intrinsic.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_buf_UAV_buf"}, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %CB_cbuffer"}, + "Non-UAV destination to atomic intrinsic.", + false); + + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_buf_UAV_buf"}, + {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %\"$Globals_cbuffer\""}, + "Non-UAV destination to atomic intrinsic.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_buf_UAV_buf"}, + {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %\"$Globals_cbuffer\""}, + "Non-UAV destination to atomic intrinsic.", + false); + + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"atomicrmw add i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, + {"atomicrmw add i32 addrspace(3)* @\"\\01?cgs_var@@3IB\""}, + "Constant destination to atomic.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"cmpxchg i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, + {"cmpxchg i32 addrspace(3)* @\"\\01?cgs_var@@3IB\""}, + "Constant destination to atomic.", + false); + +} + +// Check validation error for non-groupshared dest +TEST_F(ValidationTest, AtomicsInvalidDests) { + if (m_ver.SkipDxilVersion(1, 7)) return; + std::vector pArguments = { L"-HV", L"2021", L"-Zi" }; + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "lib_6_3", + pArguments.data(), 2, nullptr, 0, + {"atomicrmw add i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, + {"atomicrmw add i32* %res"}, + "Non-groupshared destination to atomic operation.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "lib_6_3", + pArguments.data(), 2, nullptr, 0, + {"cmpxchg i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, + {"cmpxchg i32* %res"}, + "Non-groupshared destination to atomic operation.", + false); + +} diff --git a/utils/hct/hctdb.py b/utils/hct/hctdb.py index 3aa8c0b354..955cfe5f8c 100644 --- a/utils/hct/hctdb.py +++ b/utils/hct/hctdb.py @@ -2623,6 +2623,9 @@ def build_valrules(self): self.add_valrule("Instr.MultipleGetMeshPayload", "GetMeshPayload cannot be called multiple times.") self.add_valrule("Instr.NotOnceDispatchMesh", "DispatchMesh must be called exactly once in an Amplification shader.") self.add_valrule("Instr.NonDominatingDispatchMesh", "Non-Dominating DispatchMesh call.") + self.add_valrule("Instr.AtomicOpNonGroupshared", "Non-groupshared destination to atomic operation.") + self.add_valrule("Instr.AtomicIntrinNonUAV", "Non-UAV destination to atomic intrinsic.") + self.add_valrule("Instr.AtomicConst", "Constant destination to atomic.") # Some legacy rules: # - space is only supported for shader targets 5.1 and higher From 192090e1ece1a32f062b50c93aebd8004c24960b Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Wed, 11 May 2022 22:57:28 -0700 Subject: [PATCH 02/10] simplify atomic verifier shader --- tools/clang/test/DXILValidation/atomics.hlsl | 50 +++----------------- 1 file changed, 6 insertions(+), 44 deletions(-) diff --git a/tools/clang/test/DXILValidation/atomics.hlsl b/tools/clang/test/DXILValidation/atomics.hlsl index 9a034f92ee..0aca93be27 100644 --- a/tools/clang/test/DXILValidation/atomics.hlsl +++ b/tools/clang/test/DXILValidation/atomics.hlsl @@ -1,58 +1,20 @@ -// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX=[0] %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX=[0] %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX=[0] %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=gs_var -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=gs_arr -DIDX=[0] %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=cb_var -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=cb_arr -DIDX=[0] %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=sc_var -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=sc_arr -DIDX=[0] %s | %FileCheck %s -// These two are different because they aren't const, so are caught later -// RUN: %dxc -T cs_6_0 -DDEST=loc_var -DIDX= %s | %FileCheck %s -check-prefix=CHKLOC -// RUN: %dxc -T cs_6_0 -DDEST=loc_arr -DIDX=[0] %s | %FileCheck %s -check-prefix=CHKLOC - - -// Test various Interlocked ops using different invalid destination memory types -// The way the dest param of atomic ops is lowered is unique and missed a lot of -// these invalid uses. There are a few points where the lowering branches depending -// on the memory type, so this tries to cover all those branches: -// groupshared, cbuffers, structbuffers, other resources, and other non-resources +// Validation template including valid operations and invalid destinations +// that the validator test will swap in to ensure that the validator produces +// appropriate errors. // Valid resources for atomics to create valid output that will later be manipulated to test the validator RWStructuredBuffer rw_structbuf; RWBuffer rw_buf; RWTexture1D rw_tex; +// SRVs to plug into atomics StructuredBuffer ro_structbuf; Buffer ro_buf; Texture1D ro_tex; -RWBuffer rw_bufswiz; -RWTexture1D rw_texswiz; - -struct TexCoords { - uint s, t, r, q; -}; - -RWBuffer rw_bufmemb; - -struct TexCoordsBits { - uint s : 8; - uint t : 8; - uint r : 8; - uint q : 8; -}; - -RWBuffer rw_bufbits; -RWStructuredBuffer rw_strbits; - const groupshared uint cgs_var; groupshared uint gs_var; -groupshared TexCoordsBits gs_bits; RWStructuredBuffer output; // just something to keep the variables alive @@ -86,6 +48,6 @@ void main(uint ix : SV_GroupIndex) { InterlockedCompareStore(gs_var, 1, 2); // Token usages of the invalid resources and variables so they are available in the output - output[ix] = ix + cb_var + cb_gvar + rw_bufbits[ix].s + rw_strbits[ix].s + cgs_var + gs_bits.q + - rw_bufswiz[ix].x + rw_texswiz[ix].x + ro_structbuf[ix] + ro_buf[ix] + ro_tex[ix] + rw_bufmemb[ix].s; + output[ix] = ix + cb_var + cb_gvar + cgs_var + + ro_structbuf[ix] + ro_buf[ix] + ro_tex[ix]; } From 1cd4f132354cdcfb49b86eebf315519ee6bd6b14 Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Thu, 12 May 2022 16:08:03 -0700 Subject: [PATCH 03/10] add lib test, refine restype test add test for reviewer commented lib export func test remove unintended and fairly weird unsubscripted references to resources in restypes test add more specific checks for unfound atomics for resources in restypes --- .../intrinsics/atomic/atomic_restypes.hlsl | 39 +++++++------- .../atomic/atomic_restypes_lib.hlsl | 52 +++++++++++++++++++ 2 files changed, 70 insertions(+), 21 deletions(-) create mode 100644 tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes_lib.hlsl diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl index bb43b060fb..4ceff7dd42 100644 --- a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl +++ b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl @@ -1,9 +1,6 @@ -// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX=[0] %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX=[0] %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX= %s | %FileCheck %s -// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX=[0] %s | %FileCheck %s +// RUN: %dxc -T cs_6_0 -DDEST=ro_structbuf -DIDX=[0] %s | %FileCheck %s -check-prefixes=CHKRES,CHECK +// RUN: %dxc -T cs_6_0 -DDEST=ro_buf -DIDX=[0] %s | %FileCheck %s -check-prefixes=CHKRES,CHECK +// RUN: %dxc -T cs_6_0 -DDEST=ro_tex -DIDX=[0] %s | %FileCheck %s -check-prefixes=CHKRES,CHECK // RUN: %dxc -T cs_6_0 -DDEST=gs_var -DIDX= %s | %FileCheck %s // RUN: %dxc -T cs_6_0 -DDEST=gs_arr -DIDX=[0] %s | %FileCheck %s // RUN: %dxc -T cs_6_0 -DDEST=cb_var -DIDX= %s | %FileCheck %s @@ -51,9 +48,9 @@ void main(uint ix : SV_GroupIndex) { // add // CHECK: error: no matching function for call to 'InterlockedAdd' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHECK: error: no matching function for call to 'InterlockedAdd' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedAdd(DEST IDX, val); @@ -61,9 +58,9 @@ void main(uint ix : SV_GroupIndex) { // min // CHECK: error: no matching function for call to 'InterlockedMin' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHECK: error: no matching function for call to 'InterlockedMin' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedMin(DEST IDX, val); @@ -71,9 +68,9 @@ void main(uint ix : SV_GroupIndex) { // max // CHECK: error: no matching function for call to 'InterlockedMax' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHECK: error: no matching function for call to 'InterlockedMax' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedMax(DEST IDX, val); @@ -81,9 +78,9 @@ void main(uint ix : SV_GroupIndex) { // and // CHECK: error: no matching function for call to 'InterlockedAnd' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHECK: error: no matching function for call to 'InterlockedAnd' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedAnd(DEST IDX, val); @@ -91,9 +88,9 @@ void main(uint ix : SV_GroupIndex) { // or // CHECK: error: no matching function for call to 'InterlockedOr' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHECK: error: no matching function for call to 'InterlockedOr' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedOr(DEST IDX, val); @@ -101,9 +98,9 @@ void main(uint ix : SV_GroupIndex) { // xor // CHECK: error: no matching function for call to 'InterlockedXor' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHECK: error: no matching function for call to 'InterlockedXor' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedXor(DEST IDX, val); @@ -111,19 +108,19 @@ void main(uint ix : SV_GroupIndex) { // compareStore // CHECK: error: no matching function for call to 'InterlockedCompareStore' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedCompareStore(DEST IDX, comp, val); // exchange // CHECK: error: no matching function for call to 'InterlockedExchange' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedExchange(DEST IDX, val, orig); // compareExchange // CHECK: error: no matching function for call to 'InterlockedCompareExchange' - // CHECK: note: candidate function not viable: no known conversion from + // CHKRES: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier // CHKLOC: error: Atomic operation targets must be groupshared or UAV InterlockedCompareExchange(DEST IDX, comp, val, orig); diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes_lib.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes_lib.hlsl new file mode 100644 index 0000000000..f2906346b3 --- /dev/null +++ b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes_lib.hlsl @@ -0,0 +1,52 @@ +// RUN: %dxc -T lib_6_3 -DDEST=ro_structbuf %s | %FileCheck %s +// RUN: %dxc -T lib_6_3 -DDEST=ro_structbuf %s | %FileCheck %s +// RUN: %dxc -T lib_6_3 -DDEST=ro_buf %s | %FileCheck %s +// RUN: %dxc -T lib_6_3 -DDEST=ro_buf %s | %FileCheck %s +// RUN: %dxc -T lib_6_3 -DDEST=ro_tex %s | %FileCheck %s +// RUN: %dxc -T lib_6_3 -DDEST=ro_tex %s | %FileCheck %s + + +// Test that errors on atomic dest params will still fire when used in exported +// functions in a library shader. Limits testing to one each of binop and xchg + +StructuredBuffer ro_structbuf; +Buffer ro_buf; +Texture1D ro_tex; + +RWStructuredBuffer output; // just something to keep the variables live + +// CHECK: error: no matching function for call to 'InterlockedAdd' +// CHECK: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier +// CHECK: error: no matching function for call to 'InterlockedAdd' +// CHECK: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier +// CHECK: error: no matching function for call to 'InterlockedAdd' +// CHECK: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier +export void AtomicAdd(StructuredBuffer res, uint val) {InterlockedAdd(res[0], val);} +export void AtomicAdd(Buffer res, uint val) {InterlockedAdd(res[0], val);} +export void AtomicAdd(Texture1D res, uint val) {InterlockedAdd(res[0], val);} + +// CHECK: error: no matching function for call to 'InterlockedCompareStore' +// CHECK: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier +// CHECK: error: no matching function for call to 'InterlockedCompareStore' +// CHECK: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier +// CHECK: error: no matching function for call to 'InterlockedCompareStore' +// CHECK: note: candidate function not viable: 1st argument {{.*}} would lose const qualifier +export void AtomicCompareStore(StructuredBuffer res, uint comp, uint val) {InterlockedCompareStore(res[0], comp, val);} +export void AtomicCompareStore(Buffer res, uint comp, uint val) {InterlockedCompareStore(res[0], comp, val);} +export void AtomicCompareStore(Texture1D res, uint comp, uint val) {InterlockedCompareStore(res[0], comp, val);} + +[numthreads(1,1,1)] +void main(uint ix : SV_GroupIndex) { + + int val = 1; + uint comp = 1; + uint orig; + + // Add + AtomicAdd(DEST, val); + + // CompareStore + AtomicCompareStore(DEST, comp, val); + + output[ix] = (float)DEST[0]; +} From 9e9cdcbabc981f494e572acf7ef9a50904510ef0 Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Fri, 13 May 2022 18:28:27 -0700 Subject: [PATCH 04/10] repond to feedback Add location information for overload mismatch notes. Their absence was preventing verifier tests from working. For note referring to declaration location, skip if ther is no location due to being builtin Convert all new error tests to verifier tests Reword an erorr message revise test to be valid HLSL --- lib/HLSL/HLOperationLower.cpp | 2 +- tools/clang/lib/Sema/SemaOverload.cpp | 2 +- tools/clang/test/DXILValidation/atomics.hlsl | 2 +- .../clang/test/HLSL/atomics-on-bitfields.hlsl | 30 +++++++++ tools/clang/test/HLSL/wave.hlsl | 8 +-- tools/clang/test/HLSL/write-const-arrays.hlsl | 50 +++++++++++++++ .../errors/write_const_arrays.hlsl | 61 ------------------- .../atomic/atomic_on_bitfields.hlsl | 36 ----------- tools/clang/unittests/HLSL/VerifierTest.cpp | 10 +++ 9 files changed, 97 insertions(+), 104 deletions(-) create mode 100644 tools/clang/test/HLSL/atomics-on-bitfields.hlsl create mode 100644 tools/clang/test/HLSL/write-const-arrays.hlsl delete mode 100644 tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/write_const_arrays.hlsl delete mode 100644 tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_bitfields.hlsl diff --git a/lib/HLSL/HLOperationLower.cpp b/lib/HLSL/HLOperationLower.cpp index 7c9ead79e5..71bd94e95b 100644 --- a/lib/HLSL/HLOperationLower.cpp +++ b/lib/HLSL/HLOperationLower.cpp @@ -7751,7 +7751,7 @@ void TranslateDefaultSubscript(CallInst *CI, HLOperationLowerHelper &helper, HL if (RC == DXIL::ResourceClass::SRV) { // Invalid operations. Translated = false; - dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer."); + dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on SRV."); return; } switch (IOP) { diff --git a/tools/clang/lib/Sema/SemaOverload.cpp b/tools/clang/lib/Sema/SemaOverload.cpp index 0248f3c718..6c3e4aa5e8 100644 --- a/tools/clang/lib/Sema/SemaOverload.cpp +++ b/tools/clang/lib/Sema/SemaOverload.cpp @@ -11020,7 +11020,7 @@ static ExprResult FinishOverloadedCallExpr(Sema &SemaRef, Scope *S, Expr *Fn, SemaRef.Diag(Fn->getLocStart(), diag::err_ovl_no_viable_function_in_call) << ULE->getName() << Fn->getSourceRange(); - CandidateSet->NoteCandidates(SemaRef, OCD_AllCandidates, Args); + CandidateSet->NoteCandidates(SemaRef, OCD_AllCandidates, Args, StringRef(), ULE->getLocStart()); // HLSL Change - add loc break; } diff --git a/tools/clang/test/DXILValidation/atomics.hlsl b/tools/clang/test/DXILValidation/atomics.hlsl index 0aca93be27..3dd2e38b41 100644 --- a/tools/clang/test/DXILValidation/atomics.hlsl +++ b/tools/clang/test/DXILValidation/atomics.hlsl @@ -12,7 +12,7 @@ StructuredBuffer ro_structbuf; Buffer ro_buf; Texture1D ro_tex; -const groupshared uint cgs_var; +const groupshared uint cgs_var = 0; groupshared uint gs_var; diff --git a/tools/clang/test/HLSL/atomics-on-bitfields.hlsl b/tools/clang/test/HLSL/atomics-on-bitfields.hlsl new file mode 100644 index 0000000000..5ef0800e0c --- /dev/null +++ b/tools/clang/test/HLSL/atomics-on-bitfields.hlsl @@ -0,0 +1,30 @@ +// RUN: %clang_cc1 -HV 2021 -fsyntax-only -ffreestanding -verify %s + +// Ensure that atomic operations fail when used with bitfields +// Use structured and typed buffers as the resources that can use structs +// and also both binary op and exchange atomic ops as either difference +// can cause the compiler to take different paths + +struct TexCoords { + uint s : 8; + uint t : 8; + uint r : 8; + uint q : 8; +}; + +RWBuffer buf; +RWStructuredBuffer str; +groupshared TexCoords gs; + +[numthreads(8,8,1)] +void main( uint2 tid : SV_DispatchThreadID) { + + InterlockedOr(buf[tid.y].q, 2); /* expected-error {{no matching function for call to 'InterlockedOr'}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned int &' for 1st argument}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned long long &' for 1st argument}} */ + InterlockedCompareStore(buf[tid.y].q, 3, 1); /* expected-error {{no matching function for call to 'InterlockedCompareStore'}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned int &' for 1st argument}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned long long &' for 1st argument}} */ + + InterlockedOr(str[tid.y].q, 2); /* expected-error {{no matching function for call to 'InterlockedOr'}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned int' for 1st argument}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned long long' for 1st argument}} */ + InterlockedCompareStore(str[tid.y].q, 3, 1); /* expected-error {{no matching function for call to 'InterlockedCompareStore'}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned int' for 1st argument}} expected-note {{candidate function not viable: no known conversion from 'uint' to 'unsigned long long' for 1st argument}} */ + + InterlockedOr(gs.q, 2); /* expected-error {{no matching function for call to 'InterlockedOr'}} expected-note {{candidate function not viable: 1st argument ('__attribute__((address_space(3))) uint') is in address space 3, but parameter must be in address space 0}} expected-note {{candidate function not viable: no known conversion from '__attribute__((address_space(3))) uint' to 'unsigned long long' for 1st argument}} */ + InterlockedCompareStore(gs.q, 3, 1); /* expected-error {{no matching function for call to 'InterlockedCompareStore'}} expected-note {{candidate function not viable: 1st argument ('__attribute__((address_space(3))) uint') is in address space 3, but parameter must be in address space 0}} expected-note {{candidate function not viable: no known conversion from '__attribute__((address_space(3))) uint' to 'unsigned long long' for 1st argument}} */ +} diff --git a/tools/clang/test/HLSL/wave.hlsl b/tools/clang/test/HLSL/wave.hlsl index a8e8adf88f..0357d4286b 100644 --- a/tools/clang/test/HLSL/wave.hlsl +++ b/tools/clang/test/HLSL/wave.hlsl @@ -14,7 +14,7 @@ float4 main() : SV_Target { // Divergent, single thread executing here. } uint a = WaveGetLaneIndex(); - + if (WaveGetLaneCount() == 0) { // Unlikely! } @@ -44,9 +44,9 @@ float4 main() : SV_Target { // expected-note@? {{candidate function}} // expected-note@? {{candidate function}} // expected-note@? {{candidate function}} - WaveActiveBitAnd(f1); // expected-error {{no matching function for call to 'WaveActiveBitAnd'}} - WaveActiveBitOr(f1); // expected-error {{no matching function for call to 'WaveActiveBitOr'}} - WaveActiveBitXor(f1); // expected-error {{no matching function for call to 'WaveActiveBitXor'}} + WaveActiveBitAnd(f1); // expected-error {{no matching function for call to 'WaveActiveBitAnd'}} expected-note {{candidate function not viable: no known conversion from 'float' to 'unsigned int' for 1st argument}} + WaveActiveBitOr(f1); // expected-error {{no matching function for call to 'WaveActiveBitOr'}} expected-note {{candidate function not viable: no known conversion from 'float' to 'unsigned int' for 1st argument}} + WaveActiveBitXor(f1); // expected-error {{no matching function for call to 'WaveActiveBitXor'}} expected-note {{candidate function not viable: no known conversion from 'float' to 'unsigned int' for 1st argument}} u3 = WaveActiveBitAnd(u3); u3 = WaveActiveBitOr(u3); u3 = WaveActiveBitXor(u3); diff --git a/tools/clang/test/HLSL/write-const-arrays.hlsl b/tools/clang/test/HLSL/write-const-arrays.hlsl new file mode 100644 index 0000000000..2f635ae3ee --- /dev/null +++ b/tools/clang/test/HLSL/write-const-arrays.hlsl @@ -0,0 +1,50 @@ +// RUN: %clang_cc1 -fsyntax-only -ffreestanding -verify %s + +// Array subscripts lost their qualifiers including const due to taking a different +// path for HLSL on account of having no array to ptr decay +// This test verifies that subscripts of constant and cbuf arrays can't be assigned +// or otherwise altered by a few mechanisms + +/* Expected note with no locations (implicit built-in): + expected-note@? {{function 'operator[]' which returns const-qualified type 'const unsigned int &' declared here}} +*/ + +StructuredBuffer g_robuf; +Texture1D g_tex; +uint g_cbuf[4]; + +const groupshared uint gs_val[4]; + +[NumThreads(1, 1, 1)] +void main() { + const uint local[4]; + + // Assigning using assignment operator + local[0] = 0; /* expected-error {{read-only variable is not assignable}} */ + gs_val[0] = 0; /* expected-error {{read-only variable is not assignable}} */ + g_cbuf[0] = 0; /* expected-error {{read-only variable is not assignable}} */ + g_robuf[0] = 0; /* expected-error {{cannot assign to return value because function 'operator[]' returns a const value}} */ + + // Assigning using out param of builtin function + double d = 1.0; + asuint(d, local[0], local[1]); /* expected-error {{no matching function for call to 'asuint'}} expected-note {{candidate function not viable: 2nd argument ('const uint') would lose const qualifier}} */ + asuint(d, gs_val[0], gs_val[1]); /* expected-error {{no matching function for call to 'asuint'}} expected-note {{candidate function not viable: 2nd argument ('const uint') would lose const qualifier}} */ + asuint(d, g_cbuf[0], g_cbuf[1]); /* expected-error {{no matching function for call to 'asuint'}} expected-note {{candidate function not viable: 2nd argument ('const uint') would lose const qualifier}} */ + asuint(d, g_robuf[0], g_robuf[1]); /* expected-error {{no matching function for call to 'asuint'}} expected-note {{candidate function not viable: 2nd argument ('const unsigned int') would lose const qualifier}} */ + + + // Assigning using out param of method + g_tex.GetDimensions(local[0]); /* expected-error {{no matching member function for call to 'GetDimensions'}} expected-note {{candidate function template not viable: requires 3 arguments, but 1 was provided}} */ + g_tex.GetDimensions(gs_val[0]); /* expected-error {{no matching member function for call to 'GetDimensions'}} expected-note {{candidate function template not viable: requires 3 arguments, but 1 was provided}} */ + g_tex.GetDimensions(g_cbuf[0]); /* expected-error {{no matching member function for call to 'GetDimensions'}} expected-note {{candidate function template not viable: requires 3 arguments, but 1 was provided}} */ + g_tex.GetDimensions(g_robuf[0]); /* expected-error {{no matching member function for call to 'GetDimensions'}} expected-note {{candidate function template not viable: requires 3 arguments, but 1 was provided}} */ + + + // Assigning using dest param of atomics + // Distinct because of special handling of atomics dest param + InterlockedAdd(local[0], 1); /* expected-error {{no matching function for call to 'InterlockedAdd'}} expected-note {{candidate function not viable: 1st argument ('const uint') would lose const qualifier}} expected-note {{candidate function not viable: no known conversion from 'const uint' to 'unsigned long long &' for 1st argument}} */ + InterlockedAdd(gs_val[0], 1); /* expected-error {{no matching function for call to 'InterlockedAdd'}} expected-note {{candidate function not viable: 1st argument ('const uint') would lose const qualifier}} expected-note {{candidate function not viable: no known conversion from 'const uint' to 'unsigned long long' for 1st argument}} */ + InterlockedAdd(g_cbuf[0], 1); /* expected-error {{no matching function for call to 'InterlockedAdd'}} expected-note {{candidate function not viable: 1st argument ('const uint') would lose const qualifier}} expected-note {{candidate function not viable: no known conversion from 'const uint' to 'unsigned long long' for 1st argument}} */ + InterlockedAdd(g_robuf[0], 1); /* expected-error {{no matching function for call to 'InterlockedAdd'}} expected-note {{candidate function not viable: 1st argument ('const unsigned int') would lose const qualifier}} expected-note {{candidate function not viable: no known conversion from 'const unsigned int' to 'unsigned long long' for 1st argument}} */ + +} diff --git a/tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/write_const_arrays.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/write_const_arrays.hlsl deleted file mode 100644 index a445fe73af..0000000000 --- a/tools/clang/test/HLSLFileCheck/hlsl/diagnostics/errors/write_const_arrays.hlsl +++ /dev/null @@ -1,61 +0,0 @@ -// RUN: %dxc -T cs_6_0 _HV 2021 %s | FileCheck %s - -// Array subscripts lost their qualifiers including const due to taking a different -// path for HLSL on account of having no array to ptr decay -// This test verifies that subscripts of constant and cbuf arrays can't be assigned -// or otherwise altered by a few mechanisms - -StructuredBuffer g_robuf; -Texture1D g_tex; -uint g_cbuf[4]; - -const groupshared uint gs_val[4]; - -[NumThreads(1, 1, 1)] -void main() { - const uint local[4]; - - //CHECK: error: read-only variable is not assignable - //CHECK: error: read-only variable is not assignable - //CHECK: error: read-only variable is not assignable - //CHECK: error: cannot assign to return value - // Assigning using assignment operator - local[0] = 0; - gs_val[0] = 0; - g_cbuf[0] = 0; - g_robuf[0] = 0; - - // Assigning using out param of builtin function - //CHECK: error: no matching function for call to 'sincos' - //CHECK: error: no matching function for call to 'sincos' - //CHECK: error: no matching function for call to 'sincos' - //CHECK: error: no matching function for call to 'sincos' - sincos(0.0, local[0], local[0]); - sincos(0.0, gs_val[0], gs_val[0]); - sincos(0.0, g_cbuf[0], g_cbuf[0]); - sincos(0.0, g_robuf[0], g_robuf[0]); - - - // Assigning using out param of method - //CHECK: error: no matching member function for call to 'GetDimensions' - //CHECK: error: no matching member function for call to 'GetDimensions' - //CHECK: error: no matching member function for call to 'GetDimensions' - //CHECK: error: no matching member function for call to 'GetDimensions' - g_tex.GetDimensions(local[0]); - g_tex.GetDimensions(gs_val[0]); - g_tex.GetDimensions(g_cbuf[0]); - g_tex.GetDimensions(g_robuf[0]); - - - // Assigning using dest param of atomics - // Distinct because of special handling of atomics dest param - //CHECK: error: no matching function for call to 'InterlockedAdd' - //CHECK: error: no matching function for call to 'InterlockedAdd' - //CHECK: error: no matching function for call to 'InterlockedAdd' - //CHECK: error: no matching function for call to 'InterlockedAdd' - InterlockedAdd(local[0], 1); - InterlockedAdd(gs_val[0], 1); - InterlockedAdd(g_cbuf[0], 1); - InterlockedAdd(g_robuf[0], 1); - -} diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_bitfields.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_bitfields.hlsl deleted file mode 100644 index 643fce1511..0000000000 --- a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_bitfields.hlsl +++ /dev/null @@ -1,36 +0,0 @@ -// RUN: %dxc -HV 2021 -T cs_6_0 %s | FileCheck %s - -// Ensure that atomic operations fail when used with bitfields -// Use structured and typed buffers as the resources that can use structs -// and also both binary op and exchange atomic ops as either difference -// can cause the compiler to take different paths - -struct TexCoords { - uint s : 8; - uint t : 8; - uint r : 8; - uint q : 8; -}; - -RWBuffer buf; -RWStructuredBuffer str; -groupshared TexCoords gs; - -[numthreads(8,8,1)] -void main( uint2 tid : SV_DispatchThreadID) { - - // CHECK: error: no matching function for call to 'InterlockedOr' - // CHECK: error: no matching function for call to 'InterlockedCompareStore' - InterlockedOr(buf[tid.y].q, 2); - InterlockedCompareStore(buf[tid.y].q, 3, 1); - - // CHECK: error: no matching function for call to 'InterlockedOr' - // CHECK: error: no matching function for call to 'InterlockedCompareStore' - InterlockedOr(str[tid.y].q, 2); - InterlockedCompareStore(str[tid.y].q, 3, 1); - - // CHECK: error: no matching function for call to 'InterlockedOr' - // CHECK: error: no matching function for call to 'InterlockedCompareStore' - InterlockedOr(gs.q, 2); - InterlockedCompareStore(gs.q, 3, 1); -} diff --git a/tools/clang/unittests/HLSL/VerifierTest.cpp b/tools/clang/unittests/HLSL/VerifierTest.cpp index 15da1f3365..bb7f07eb7d 100644 --- a/tools/clang/unittests/HLSL/VerifierTest.cpp +++ b/tools/clang/unittests/HLSL/VerifierTest.cpp @@ -97,6 +97,8 @@ class VerifierTest TEST_CLASS_DERIVATION { TEST_METHOD(RunVectorOr) TEST_METHOD(RunArrayConstAssign) TEST_METHOD(RunInputPatchConst) + TEST_METHOD(RunWriteConstArrays) + TEST_METHOD(RunAtomicsOnBitfields) void CheckVerifies(const wchar_t* path) { WEX::TestExecution::SetVerifyOutput verifySettings(WEX::TestExecution::VerifyOutputSettings::LogOnlyFailures); @@ -419,3 +421,11 @@ TEST_F(VerifierTest, RunArrayConstAssign) { TEST_F(VerifierTest, RunInputPatchConst) { CheckVerifiesHLSL(L"InputPatch-const.hlsl"); } + +TEST_F(VerifierTest, RunWriteConstArrays) { + CheckVerifiesHLSL(L"write-const-arrays.hlsl"); +} + +TEST_F(VerifierTest, RunAtomicsOnBitfields) { + CheckVerifiesHLSL(L"atomics-on-bitfields.hlsl"); +} From fd37bb2700e68c1fb6a33ff155e99f57d2d5d303 Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Sun, 15 May 2022 20:24:13 -0700 Subject: [PATCH 05/10] Finish responding to feedback Add drilling through GEP and BC for groupshared atomic validation Add validation test for const groupshared atomic used through a GEP Enhance error detection for atomics at lowering time by accounting for any sequence of GEP and subscript operations that might end with a valid resource. Add various new testing for atomic operations performed on destination values that are attached to a string of GEP and subscripts. --- lib/HLSL/DxilValidation.cpp | 19 ++++- lib/HLSL/HLOperationLower.cpp | 36 +++++----- tools/clang/lib/Sema/SemaHLSL.cpp | 2 +- tools/clang/test/DXILValidation/atomics.hlsl | 10 +-- tools/clang/test/HLSL/wave.hlsl | 3 - .../intrinsics/atomic/atomic_on_members.hlsl | 69 +++++++++++++++++-- .../atomic/atomic_structuredbuf_i64.hlsl | 14 +++- tools/clang/unittests/HLSL/ValidationTest.cpp | 13 ++++ 8 files changed, 131 insertions(+), 35 deletions(-) diff --git a/lib/HLSL/DxilValidation.cpp b/lib/HLSL/DxilValidation.cpp index 0d19ceb5ad..5c1fe5e80d 100644 --- a/lib/HLSL/DxilValidation.cpp +++ b/lib/HLSL/DxilValidation.cpp @@ -3191,9 +3191,24 @@ static void ValidateFunctionBody(Function *F, ValidationContext &ValCtx) { if ((elType->isIntegerTy(64)) && !pSM->IsSM66Plus()) ValCtx.EmitInstrFormatError(&I, ValidationRule::SmOpcodeInInvalidFunction, {"64-bit atomic operations", "Shader Model 6.6+"}); - if (ptrType->getAddressSpace() != DXIL::kTGSMAddrSpace) { + + if (ptrType->getAddressSpace() != DXIL::kTGSMAddrSpace) ValCtx.EmitInstrError(&I, ValidationRule::InstrAtomicOpNonGroupshared); - } else if (GlobalVariable *GV = dyn_cast(Ptr)) { + + // Drill through GEP and bitcasts + while (true) { + if (GEPOperator *GEP = dyn_cast(Ptr)) { + Ptr = GEP->getPointerOperand(); + continue; + } + if (BitCastInst *BC = dyn_cast(Ptr)) { + Ptr = BC->getOperand(0); + continue; + } + break; + } + + if (GlobalVariable *GV = dyn_cast(Ptr)) { if(GV->isConstant()) ValCtx.EmitInstrError(&I, ValidationRule::InstrAtomicConst); } diff --git a/lib/HLSL/HLOperationLower.cpp b/lib/HLSL/HLOperationLower.cpp index 71bd94e95b..fb7279dbcb 100644 --- a/lib/HLSL/HLOperationLower.cpp +++ b/lib/HLSL/HLOperationLower.cpp @@ -4424,29 +4424,33 @@ static Value* SkipAddrSpaceCast(Value* Ptr) { // For known non-groupshared, verify that the destination param is valid void ValidateAtomicDestination(CallInst *CI, HLObjectOperationLowerHelper *pObjHelper) { Value *dest = CI->getArgOperand(HLOperandIndex::kInterlockedDestOpIndex); - GetElementPtrInst *gep = nullptr; - // If we encounter a gep, we may provide a more specific error message - while (isa(dest)) { - gep = cast(dest); - dest = gep->getPointerOperand(); - } + bool hasGep = isa(dest); + + // Confirm that dest is a properly-used UAV - // Confirm that dest is a UAV - if (CallInst *destCall = dyn_cast(dest)) { - hlsl::HLOpcodeGroup group = hlsl::GetHLOpcodeGroup(destCall->getCalledFunction()); - while (group == HLOpcodeGroup::HLSubscript) { - dest = destCall->getArgOperand(HLOperandIndex::kSubscriptObjectOpIdx); - if (!isa(dest)) + // Drill through subscripts and geps, anything else indicates a misuse + while(true) { + if (GetElementPtrInst *gep = dyn_cast(dest)) { + dest = gep->getPointerOperand(); + continue; + } + if (CallInst *handle = dyn_cast(dest)) { + hlsl::HLOpcodeGroup group = hlsl::GetHLOpcodeGroup(handle->getCalledFunction()); + if (group != HLOpcodeGroup::HLSubscript) break; - destCall = cast(dest); - group = hlsl::GetHLOpcodeGroup(destCall->getCalledFunction()); + dest = handle->getArgOperand(HLOperandIndex::kSubscriptObjectOpIdx); + continue; } - DXIL::ResourceKind RK = pObjHelper->GetRK(destCall); + break; + } + + if(pObjHelper->GetRC(dest) == DXIL::ResourceClass::UAV) { + DXIL::ResourceKind RK = pObjHelper->GetRK(dest); if (DXIL::IsStructuredBuffer(RK)) return; // no errors if (DXIL::IsTyped(RK)) { - if (gep) + if (hasGep) dxilutil::EmitErrorOnInstruction(CI, "Typed resources used in atomic operations must have a scalar element type."); return; // error emitted or else no errors } diff --git a/tools/clang/lib/Sema/SemaHLSL.cpp b/tools/clang/lib/Sema/SemaHLSL.cpp index 8659290c38..7b9aa93064 100644 --- a/tools/clang/lib/Sema/SemaHLSL.cpp +++ b/tools/clang/lib/Sema/SemaHLSL.cpp @@ -6167,7 +6167,7 @@ bool HLSLExternalSource::MatchArguments( // Catch invalid atomic dest parameters if (iArg == kAtomicDstOperandIdx && IsAtomicOperation(static_cast(pIntrinsic->Op))) { - // bitfield error is confusing + // This produces an error for bitfields that is a bit confusing because it says uint can't cast to uint if (pType.isConstant(actx) || pCallArg->getObjectKind() == OK_BitField) { badArgIdx = std::min(badArgIdx, iArg); } diff --git a/tools/clang/test/DXILValidation/atomics.hlsl b/tools/clang/test/DXILValidation/atomics.hlsl index 3dd2e38b41..1df3a2490a 100644 --- a/tools/clang/test/DXILValidation/atomics.hlsl +++ b/tools/clang/test/DXILValidation/atomics.hlsl @@ -13,10 +13,11 @@ Buffer ro_buf; Texture1D ro_tex; const groupshared uint cgs_var = 0; +const groupshared uint cgs_arr[3] = {0, 0, 0}; groupshared uint gs_var; -RWStructuredBuffer output; // just something to keep the variables alive +RWStructuredBuffer output; // just something to keep the variables alive cbuffer CB { uint cb_var; @@ -35,6 +36,9 @@ void main(uint ix : SV_GroupIndex) { uint res; init(res); + // Token usages of the invalid resources and variables so they are available in the output + res += cb_var + cb_gvar + cgs_var + ro_structbuf[ix] + ro_buf[ix] + ro_tex[ix] + cgs_arr[ix]; + InterlockedAdd(rw_structbuf[ix], 1); InterlockedCompareStore(rw_structbuf[ix], 1, 2); @@ -47,7 +51,5 @@ void main(uint ix : SV_GroupIndex) { InterlockedAdd(gs_var, 1); InterlockedCompareStore(gs_var, 1, 2); - // Token usages of the invalid resources and variables so they are available in the output - output[ix] = ix + cb_var + cb_gvar + cgs_var + - ro_structbuf[ix] + ro_buf[ix] + ro_tex[ix]; + output[ix] = res; } diff --git a/tools/clang/test/HLSL/wave.hlsl b/tools/clang/test/HLSL/wave.hlsl index 0357d4286b..7824aa289a 100644 --- a/tools/clang/test/HLSL/wave.hlsl +++ b/tools/clang/test/HLSL/wave.hlsl @@ -41,9 +41,6 @@ float4 main() : SV_Target { f3 = WaveActiveSum(f3); f3 = WaveActiveProduct(f3); // WaveActiveBit* with an invalid signature suggests the use of WaveActiveBallot instead. - // expected-note@? {{candidate function}} - // expected-note@? {{candidate function}} - // expected-note@? {{candidate function}} WaveActiveBitAnd(f1); // expected-error {{no matching function for call to 'WaveActiveBitAnd'}} expected-note {{candidate function not viable: no known conversion from 'float' to 'unsigned int' for 1st argument}} WaveActiveBitOr(f1); // expected-error {{no matching function for call to 'WaveActiveBitOr'}} expected-note {{candidate function not viable: no known conversion from 'float' to 'unsigned int' for 1st argument}} WaveActiveBitXor(f1); // expected-error {{no matching function for call to 'WaveActiveBitXor'}} expected-note {{candidate function not viable: no known conversion from 'float' to 'unsigned int' for 1st argument}} diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_members.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_members.hlsl index eab171ad9a..7f8c1f7c2b 100644 --- a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_members.hlsl +++ b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_on_members.hlsl @@ -1,22 +1,79 @@ -// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedOr %s | FileCheck %s -// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedCompareStore %s | FileCheck %s +// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedOr -DDEST=Str %s | FileCheck %s -check-prefix=CHKBIN +// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedCompareStore -DDEST=Str %s | FileCheck %s -check-prefix=CHKXCH +// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedOr -DDEST=Gs %s | FileCheck %s -check-prefix=CHKGSB +// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedCompareStore -DDEST=Gs %s | FileCheck %s -check-prefix=CHKGSX +// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedOr -DDEST=Buf %s | FileCheck %s -check-prefix=CHKERR +// RUN: %dxc -HV 2021 -T cs_6_0 -DOP=InterlockedCompareStore -DDEST=Buf %s | FileCheck %s -check-prefix=CHKERR // Ensure that atomic operations fail when used with struct members of a typed resource // The only typed resource that can use structs is RWBuffer // Use a binary op and an exchange op because they use different code paths +struct Simple { + uint i; + uint longEnding[1]; +}; + +struct Complex { + Simple s; + uint theEnd; +}; + struct TexCoords { uint s, t, r, q; }; -RWBuffer bufA; +#define _PASTE(pre, res) pre##res +#define PASTE(pre, res) _PASTE(pre, res) + +RWBuffer Buf; +RWBuffer simpBuf; +RWBuffer cplxBuf; + +RWStructuredBuffer Str; +RWStructuredBuffer simpStr; +RWStructuredBuffer cplxStr; + +groupshared TexCoords Gs[1]; +groupshared Simple simpGs[1]; +groupshared Complex cplxGs[1]; [numthreads(8,8,1)] void main( uint3 gtid : SV_GroupThreadID , uint gid : SV_GroupIndex) { uint orig = 0; - TexCoords tc = {0, 0, 0, 0}; + uint a = gid; + uint b = gtid.x; + uint c = gtid.y; + uint d = gtid.z; + + // CHKBIN: call i32 @dx.op.atomicBinOp.i32(i32 78 + // CHKBIN: call i32 @dx.op.atomicBinOp.i32(i32 78 + // CHKBIN: call i32 @dx.op.atomicBinOp.i32(i32 78 + // CHKBIN: call i32 @dx.op.atomicBinOp.i32(i32 78 + + // CHKXCH: call i32 @dx.op.atomicCompareExchange.i32(i32 79, + // CHKXCH: call i32 @dx.op.atomicCompareExchange.i32(i32 79, + // CHKXCH: call i32 @dx.op.atomicCompareExchange.i32(i32 79, + // CHKXCH: call i32 @dx.op.atomicCompareExchange.i32(i32 79, + + // CHKGSB: atomicrmw or i32 addrspace(3) + // CHKGSB: atomicrmw or i32 addrspace(3) + // CHKGSB: atomicrmw or i32 addrspace(3) + // CHKGSB: atomicrmw or i32 addrspace(3) + + // CHKGSX: cmpxchg i32 addrspace(3) + // CHKGSX: cmpxchg i32 addrspace(3) + // CHKGSX: cmpxchg i32 addrspace(3) + // CHKGSX: cmpxchg i32 addrspace(3) + + // CHKERR: error: Typed resources used in atomic operations must have a scalar element type. + // CHKERR: error: Typed resources used in atomic operations must have a scalar element type. + // CHKERR: error: Typed resources used in atomic operations must have a scalar element type. + // CHKERR: error: Typed resources used in atomic operations must have a scalar element type. - // CHECK: error: Typed resources used in atomic operations must have a scalar element type. - OP(bufA[gid].q, 2, orig); + OP( PASTE( ,DEST)[a].q, 2, orig ); + OP( PASTE(simp, DEST)[a].i, 2, orig ); + OP( PASTE(cplx, DEST)[a].s.i, 2, orig ); + OP( PASTE(cplx, DEST)[a].s.longEnding[d].x, 2, orig ); } diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_structuredbuf_i64.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_structuredbuf_i64.hlsl index b312895855..0e273fee3c 100644 --- a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_structuredbuf_i64.hlsl +++ b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_structuredbuf_i64.hlsl @@ -6,7 +6,7 @@ struct simple { bool thisVariableIsFalse; uint64_t i; - float3x1 longEnding[4]; + uint64_t3x1 longEnding[4]; }; struct complex { @@ -22,53 +22,61 @@ RWStructuredBuffer simpArrBuf; RWStructuredBuffer cplxBuf; RWStructuredBuffer cplxArrBuf; -void main( uint a : A, uint b: B, uint c :C) : SV_Target +void main( uint a : A, uint b: B, uint c :C, uint d :D) : SV_Target { int64_t liv = a + b; int64_t liv2 = 0, liv3 = 0; + uint64_t loc_arr[4]; // CHECK: call i64 @dx.op.atomicBinOp.i64 // CHECK: call i64 @dx.op.atomicBinOp.i64 // CHECK: call i64 @dx.op.atomicBinOp.i64 // CHECK: call i64 @dx.op.atomicBinOp.i64 // CHECK: call i64 @dx.op.atomicBinOp.i64 + // CHECK: call i64 @dx.op.atomicBinOp.i64 InterlockedAdd( simpBuf[a].i, liv ); InterlockedAdd( simpArrBuf[a][b].i, liv ); InterlockedAdd( cplxBuf[a].i, liv ); InterlockedAdd( cplxBuf[a].s.i, liv ); InterlockedAdd( cplxBuf[a].ss[b].i, liv ); + InterlockedAdd( cplxBuf[a].ss[b].longEnding[c][d].x, liv); // CHECK: call i64 @dx.op.atomicBinOp.i64 // CHECK: call i64 @dx.op.atomicBinOp.i64 // CHECK: call i64 @dx.op.atomicBinOp.i64 // CHECK: call i64 @dx.op.atomicBinOp.i64 // CHECK: call i64 @dx.op.atomicBinOp.i64 + // CHECK: call i64 @dx.op.atomicBinOp.i64 InterlockedExchange( simpBuf[a].i, liv, liv2 ); InterlockedExchange( simpArrBuf[a][b].i, liv2, liv ); InterlockedExchange( cplxBuf[a].i, liv, liv2 ); InterlockedExchange( cplxBuf[a].s.i, liv2, liv ); InterlockedExchange( cplxBuf[a].ss[b].i, liv, liv2 ); + InterlockedExchange( cplxBuf[a].ss[b].longEnding[c][d].x, liv, liv2); // CHECK: call i64 @dx.op.atomicCompareExchange.i64 // CHECK: call i64 @dx.op.atomicCompareExchange.i64 // CHECK: call i64 @dx.op.atomicCompareExchange.i64 // CHECK: call i64 @dx.op.atomicCompareExchange.i64 // CHECK: call i64 @dx.op.atomicCompareExchange.i64 + // CHECK: call i64 @dx.op.atomicCompareExchange.i64 InterlockedCompareStore( simpBuf[a].i, liv, liv2 ); InterlockedCompareStore( simpArrBuf[a][b].i, liv2, liv ); InterlockedCompareStore( cplxBuf[a].i, liv, liv2 ); InterlockedCompareStore( cplxBuf[a].s.i, liv2, liv ); InterlockedCompareStore( cplxBuf[a].ss[b].i, liv, liv2 ); + InterlockedCompareStore( cplxBuf[a].ss[b].longEnding[c][d].x, liv2, liv); // CHECK: call i64 @dx.op.atomicCompareExchange.i64 // CHECK: call i64 @dx.op.atomicCompareExchange.i64 // CHECK: call i64 @dx.op.atomicCompareExchange.i64 // CHECK: call i64 @dx.op.atomicCompareExchange.i64 // CHECK: call i64 @dx.op.atomicCompareExchange.i64 + // CHECK: call i64 @dx.op.atomicCompareExchange.i64 InterlockedCompareExchange( simpBuf[a].i, liv, liv2, liv3 ); InterlockedCompareExchange( simpArrBuf[a][b].i, liv2, liv3, liv ); InterlockedCompareExchange( cplxBuf[a].i, liv3, liv2, liv ); InterlockedCompareExchange( cplxBuf[a].s.i, liv2, liv, liv3 ); InterlockedCompareExchange( cplxBuf[a].ss[b].i, liv2, liv3, liv ); - + InterlockedCompareExchange( cplxBuf[a].ss[b].longEnding[c][d].x, liv3, liv, liv2 ); } diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index 7b009323fd..8876a194e0 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -3988,6 +3988,19 @@ TEST_F(ValidationTest, AtomicsConsts) { "Constant destination to atomic.", false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"atomicrmw add i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, + {"atomicrmw add i32 addrspace(3)* %arrayidx"}, + "Constant destination to atomic.", + false); + RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", + pArguments.data(), 2, nullptr, 0, + {"cmpxchg i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, + {"cmpxchg i32 addrspace(3)* %arrayidx"}, + "Constant destination to atomic.", + false); + } // Check validation error for non-groupshared dest From f1a7a61217222120e800859ac98710a8aa1a9463 Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Sun, 15 May 2022 23:20:15 -0700 Subject: [PATCH 06/10] Add Zi to get automated test to replace proper variable --- .../intrinsics/atomic/atomic_restypes.hlsl | 4 +-- tools/clang/unittests/HLSL/ValidationTest.cpp | 30 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl index 4ceff7dd42..23b87e4f4e 100644 --- a/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl +++ b/tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_restypes.hlsl @@ -23,8 +23,8 @@ StructuredBuffer ro_structbuf; Buffer ro_buf; Texture1D ro_tex; -const groupshared uint gs_var; -const groupshared uint gs_arr[4]; +const groupshared uint gs_var = 0; +const groupshared uint gs_arr[4] = {0, 0, 0, 0}; RWStructuredBuffer output; // just something to keep the variables live diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index 8876a194e0..c975b8e223 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -3909,93 +3909,93 @@ TEST_F(ValidationTest, CreateHandleNotAllowedSM66) { // Check for validation errors for various const dests to atomics TEST_F(ValidationTest, AtomicsConsts) { if (m_ver.SkipDxilVersion(1, 7)) return; - std::vector pArguments = { L"-HV", L"2021" }; + std::vector pArguments = { L"-HV", L"2021", L"-Zi"}; RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_structbuf_UAV_structbuf"}, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %ro_structbuf_texture_structbuf"}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_structbuf_UAV_structbuf"}, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %ro_structbuf_texture_structbuf"}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_buf_UAV_buf"}, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %ro_buf_texture_buf"}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_buf_UAV_buf"}, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %ro_buf_texture_buf"}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_tex_UAV_1d"}, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %ro_tex_texture_1d"}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_tex_UAV_1d"}, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %ro_tex_texture_1d"}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_buf_UAV_buf"}, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %CB_cbuffer"}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_buf_UAV_buf"}, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %CB_cbuffer"}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %rw_buf_UAV_buf"}, {"call i32 @dx.op.atomicBinOp.i32(i32 78, %dx.types.Handle %\"$Globals_cbuffer\""}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %rw_buf_UAV_buf"}, {"call i32 @dx.op.atomicCompareExchange.i32(i32 79, %dx.types.Handle %\"$Globals_cbuffer\""}, "Non-UAV destination to atomic intrinsic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"atomicrmw add i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, {"atomicrmw add i32 addrspace(3)* @\"\\01?cgs_var@@3IB\""}, "Constant destination to atomic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"cmpxchg i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, {"cmpxchg i32 addrspace(3)* @\"\\01?cgs_var@@3IB\""}, "Constant destination to atomic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"atomicrmw add i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, {"atomicrmw add i32 addrspace(3)* %arrayidx"}, "Constant destination to atomic.", false); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", - pArguments.data(), 2, nullptr, 0, + pArguments.data(), 3, nullptr, 0, {"cmpxchg i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, {"cmpxchg i32 addrspace(3)* %arrayidx"}, "Constant destination to atomic.", From a1beefde1a22a6ee94d77913f68270c81400d59a Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Mon, 16 May 2022 00:53:44 -0700 Subject: [PATCH 07/10] Make atomicsconst validation test reg name independent --- tools/clang/unittests/HLSL/ValidationTest.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index c975b8e223..93ccf267a1 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -3990,17 +3990,19 @@ TEST_F(ValidationTest, AtomicsConsts) { RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", pArguments.data(), 3, nullptr, 0, - {"atomicrmw add i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, - {"atomicrmw add i32 addrspace(3)* %arrayidx"}, + {"%([a-zA-Z0-9]+) = getelementptr \\[3 x i32\\], \\[3 x i32\\] addrspace\\(3\\)\\* @\"\\\\01\\?cgs_arr@@3QBIB\"([^\n]*)"}, + {"%\\1 = getelementptr \\[3 x i32\\], \\[3 x i32\\] addrspace\\(3\\)\\* @\"\\\\01\\?cgs_arr@@3QBIB\"\\2\n" + "%dummy = atomicrmw add i32 addrspace\\(3\\)\\* %arrayidx, i32 1 seq_cst, !dbg !104 ; line:51 col:3" + }, "Constant destination to atomic.", - false); + true); RewriteAssemblyCheckMsg(L"..\\DXILValidation\\atomics.hlsl", "cs_6_0", pArguments.data(), 3, nullptr, 0, - {"cmpxchg i32 addrspace(3)* @\"\\01?gs_var@@3IA\""}, - {"cmpxchg i32 addrspace(3)* %arrayidx"}, + {"%([a-zA-Z0-9]+) = getelementptr \\[3 x i32\\], \\[3 x i32\\] addrspace\\(3\\)\\* @\"\\\\01\\?cgs_arr@@3QBIB\"([^\n]*)"}, + {"%\\1 = getelementptr \\[3 x i32\\], \\[3 x i32\\] addrspace\\(3\\)\\* @\"\\\\01\\?cgs_arr@@3QBIB\"\\2\n" + "%dummy = cmpxchg i32 addrspace\\(3\\)\\* %\\1, i32 1, i32 2 seq_cst seq_cst, !dbg !105 ;"}, "Constant destination to atomic.", - false); - + true); } // Check validation error for non-groupshared dest From 7efb0b0918d36f144226fc0fb703147e6a17188f Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Mon, 16 May 2022 03:53:42 -0700 Subject: [PATCH 08/10] remove hack left in by mistake --- tools/clang/unittests/HLSL/ValidationTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/clang/unittests/HLSL/ValidationTest.cpp b/tools/clang/unittests/HLSL/ValidationTest.cpp index 93ccf267a1..c3c73db045 100644 --- a/tools/clang/unittests/HLSL/ValidationTest.cpp +++ b/tools/clang/unittests/HLSL/ValidationTest.cpp @@ -3992,7 +3992,7 @@ TEST_F(ValidationTest, AtomicsConsts) { pArguments.data(), 3, nullptr, 0, {"%([a-zA-Z0-9]+) = getelementptr \\[3 x i32\\], \\[3 x i32\\] addrspace\\(3\\)\\* @\"\\\\01\\?cgs_arr@@3QBIB\"([^\n]*)"}, {"%\\1 = getelementptr \\[3 x i32\\], \\[3 x i32\\] addrspace\\(3\\)\\* @\"\\\\01\\?cgs_arr@@3QBIB\"\\2\n" - "%dummy = atomicrmw add i32 addrspace\\(3\\)\\* %arrayidx, i32 1 seq_cst, !dbg !104 ; line:51 col:3" + "%dummy = atomicrmw add i32 addrspace\\(3\\)\\* %\\1, i32 1 seq_cst, !dbg !104 ; line:51 col:3" }, "Constant destination to atomic.", true); From 73049ac980191317db1d5aa0864e27c95f95be14 Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Mon, 16 May 2022 13:39:39 -0700 Subject: [PATCH 09/10] Include verifier tests in script --- utils/hct/VerifierHelper.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/utils/hct/VerifierHelper.py b/utils/hct/VerifierHelper.py index fcf526ded8..a7ca369ce4 100644 --- a/utils/hct/VerifierHelper.py +++ b/utils/hct/VerifierHelper.py @@ -112,6 +112,8 @@ 'RunVectorSyntaxExactPrecision': 'vector-syntax-exact-precision.hlsl', 'RunVectorSyntaxMix': 'vector-syntax-mix.hlsl', 'RunWave': 'wave.hlsl', + 'RunWriteConstArrays': 'write-const-arrays.hlsl', + 'RunAtomicsOnBitfields': 'atomics-on-bitfields.hlsl', } # The following test(s) do not work in fxc mode: @@ -138,6 +140,7 @@ 'RunTemplateLiteralSubstitutionFailure', 'RunVectorSyntaxExactPrecision', 'RunWave', + 'RunAtomicsOnBitfields', ] # rxRUN = re.compile(r'[ RUN ] VerifierTest.(\w+)') # gtest syntax From 35d8c06b916c63ebef03705565405aa40f6498a1 Mon Sep 17 00:00:00 2001 From: Greg Roth Date: Mon, 16 May 2022 18:12:57 -0700 Subject: [PATCH 10/10] Add some foolproofing to opcode checks in codegen Change some function signatures and local variables in SemaHLSL to try to avoid mistakenly comparing matching opcodes from different namespaces --- include/dxc/HLSL/HLOperations.h | 1 - lib/HLSL/HLOperations.cpp | 4 -- tools/clang/lib/Sema/SemaHLSL.cpp | 75 ++++++++++++++++++------------- 3 files changed, 45 insertions(+), 35 deletions(-) diff --git a/include/dxc/HLSL/HLOperations.h b/include/dxc/HLSL/HLOperations.h index 592ac8dce2..69435a92d8 100644 --- a/include/dxc/HLSL/HLOperations.h +++ b/include/dxc/HLSL/HLOperations.h @@ -132,7 +132,6 @@ void SetHLWaveSensitive(llvm::Function *F); bool IsHLWaveSensitive(llvm::Function *F); // For intrinsic opcode. -bool HasUnsignedOpcode(unsigned opcode); unsigned GetUnsignedOpcode(unsigned opcode); // For HLBinaryOpcode. bool HasUnsignedOpcode(HLBinaryOpcode opcode); diff --git a/lib/HLSL/HLOperations.cpp b/lib/HLSL/HLOperations.cpp index 66c0bbdb01..974ddd75c6 100644 --- a/lib/HLSL/HLOperations.cpp +++ b/lib/HLSL/HLOperations.cpp @@ -374,10 +374,6 @@ unsigned GetRowMajorOpcode(HLOpcodeGroup group, unsigned opcode) { } } -bool HasUnsignedOpcode(unsigned opcode) { - return HasUnsignedIntrinsicOpcode(static_cast(opcode)); -} - unsigned GetUnsignedOpcode(unsigned opcode) { return GetUnsignedIntrinsicOpcode(static_cast(opcode)); } diff --git a/tools/clang/lib/Sema/SemaHLSL.cpp b/tools/clang/lib/Sema/SemaHLSL.cpp index 7b9aa93064..d7f4b8a89c 100644 --- a/tools/clang/lib/Sema/SemaHLSL.cpp +++ b/tools/clang/lib/Sema/SemaHLSL.cpp @@ -1858,7 +1858,13 @@ static void InitParamMods(const HLSL_INTRINSIC *pIntrinsic, } } -static bool IsAtomicOperation(IntrinsicOp op) { +static bool IsBuiltinTable(LPCSTR tableName) { + return tableName == kBuiltinIntrinsicTableName; +} + +static bool IsAtomicOperation(LPCSTR tableName, IntrinsicOp op) { + if (!IsBuiltinTable(tableName)) + return false; switch (op) { case IntrinsicOp::IOP_InterlockedAdd: case IntrinsicOp::IOP_InterlockedAnd: @@ -1898,15 +1904,15 @@ static bool IsAtomicOperation(IntrinsicOp op) { } } -static bool IsBuiltinTable(LPCSTR tableName) { - return tableName == kBuiltinIntrinsicTableName; +static bool HasUnsignedOpcode(LPCSTR tableName, IntrinsicOp opcode) { + return IsBuiltinTable(tableName) && HasUnsignedIntrinsicOpcode(opcode); } static void AddHLSLIntrinsicAttr(FunctionDecl *FD, ASTContext &context, LPCSTR tableName, LPCSTR lowering, const HLSL_INTRINSIC *pIntrinsic) { - unsigned opcode = (unsigned)pIntrinsic->Op; - if (HasUnsignedOpcode(opcode) && IsBuiltinTable(tableName)) { + unsigned opcode = pIntrinsic->Op; + if (HasUnsignedOpcode(tableName, static_cast(opcode))) { QualType Ty = FD->getReturnType(); if (pIntrinsic->iOverloadParamIndex != -1) { const FunctionProtoType *FT = @@ -1965,13 +1971,11 @@ FunctionDecl *AddHLSLIntrinsicFunction( InitParamMods(pIntrinsic, paramMods); // Change dest address into reference type for atomic. - if (IsBuiltinTable(tableName)) { - if (IsAtomicOperation(static_cast(pIntrinsic->Op))) { - DXASSERT(functionArgTypeCount > kAtomicDstOperandIdx, - "else operation was misrecognized"); - functionArgQualTypes[kAtomicDstOperandIdx] = - context.getLValueReferenceType(functionArgQualTypes[kAtomicDstOperandIdx]); - } + if (IsAtomicOperation(tableName, static_cast(pIntrinsic->Op))) { + DXASSERT(functionArgTypeCount > kAtomicDstOperandIdx, + "else operation was misrecognized"); + functionArgQualTypes[kAtomicDstOperandIdx] = + context.getLValueReferenceType(functionArgQualTypes[kAtomicDstOperandIdx]); } for (size_t i = 1; i < functionArgTypeCount; i++) { @@ -2583,13 +2587,13 @@ class IntrinsicTableDefIter return _tableIndex != other._tableIndex; // More things could be compared but we only match end. } - const HLSL_INTRINSIC* operator*() + const HLSL_INTRINSIC* operator*() const { DXASSERT(_firstChecked, "otherwise deref without comparing to end"); return _tableIntrinsic; } - LPCSTR GetTableName() + LPCSTR GetTableName() const { LPCSTR tableName = nullptr; if (FAILED(_tables[_tableIndex]->GetTableName(&tableName))) { @@ -2598,7 +2602,7 @@ class IntrinsicTableDefIter return tableName; } - LPCSTR GetLoweringStrategy() + LPCSTR GetLoweringStrategy() const { LPCSTR lowering = nullptr; if (FAILED(_tables[_tableIndex]->GetLoweringStrategy(_tableIntrinsic->Op, &lowering))) { @@ -2643,17 +2647,17 @@ class IntrinsicDefIter return _current != other._current || _tableIter.operator!=(other._tableIter); } - const HLSL_INTRINSIC* operator*() + const HLSL_INTRINSIC* operator*() const { return (_current != _end) ? _current : *_tableIter; } - LPCSTR GetTableName() + LPCSTR GetTableName() const { return (_current != _end) ? kBuiltinIntrinsicTableName : _tableIter.GetTableName(); } - LPCSTR GetLoweringStrategy() + LPCSTR GetLoweringStrategy() const { return (_current != _end) ? "" : _tableIter.GetLoweringStrategy(); } @@ -4638,14 +4642,14 @@ class HLSLExternalSource : public ExternalSemaSource { } /// Attempts to match Args to the signature specification in pIntrinsic. - /// Intrinsic function to match. + /// Intrinsic function iterator. /// Type element on the class intrinsic belongs to; possibly null (eg, 'float' in 'Texture2D'). /// Invocation arguments to match. /// After exectuion, type of arguments. /// The first argument to mismatch if any /// On success, argTypes includes the clang Types to use for the signature, with the first being the return type. bool MatchArguments( - const _In_ HLSL_INTRINSIC *pIntrinsic, + const IntrinsicDefIter &cursor, _In_ QualType objectElement, _In_ QualType functionTemplateTypeArg, _In_ ArrayRef Args, @@ -4653,10 +4657,12 @@ class HLSLExternalSource : public ExternalSemaSource { _Out_ size_t &badArgIdx); /// Validate object element on intrinsic to catch case like integer on Sample. - /// Intrinsic function to validate. + /// Intrinsic function to validate. + /// Intrinsic opcode to validate. /// Type element on the class intrinsic belongs to; possibly null (eg, 'float' in 'Texture2D'). bool IsValidObjectElement( - _In_ const HLSL_INTRINSIC *pIntrinsic, + LPCSTR tableName, + _In_ IntrinsicOp op, _In_ QualType objectElement); // Returns the iterator with the first entry that matches the requirement @@ -4768,7 +4774,7 @@ class HLSLExternalSource : public ExternalSemaSource { std::vector functionArgTypes; size_t badArgIdx; - bool argsMatch = MatchArguments(pIntrinsic, QualType(), QualType(), Args, &functionArgTypes, badArgIdx); + bool argsMatch = MatchArguments(cursor, QualType(), QualType(), Args, &functionArgTypes, badArgIdx); if (!functionArgTypes.size()) return false; @@ -5900,9 +5906,11 @@ ConcreteLiteralType(Expr *litExpr, ArBasicKind kind, } _Use_decl_annotations_ bool -HLSLExternalSource::IsValidObjectElement(const HLSL_INTRINSIC *pIntrinsic, +HLSLExternalSource::IsValidObjectElement(LPCSTR tableName, const IntrinsicOp op, QualType objectElement) { - IntrinsicOp op = static_cast(pIntrinsic->Op); + // Only meant to exclude builtins, assume others are fine + if (!IsBuiltinTable(tableName)) + return true; switch (op) { case IntrinsicOp::MOP_Sample: case IntrinsicOp::MOP_SampleBias: @@ -5936,13 +5944,19 @@ HLSLExternalSource::IsValidObjectElement(const HLSL_INTRINSIC *pIntrinsic, _Use_decl_annotations_ bool HLSLExternalSource::MatchArguments( - const HLSL_INTRINSIC* pIntrinsic, + const IntrinsicDefIter &cursor, QualType objectElement, QualType functionTemplateTypeArg, ArrayRef Args, std::vector *argTypesVector, size_t &badArgIdx) { + const HLSL_INTRINSIC* pIntrinsic = *cursor; + LPCSTR tableName = cursor.GetTableName(); + IntrinsicOp builtinOp = IntrinsicOp::Num_Intrinsics; + if (IsBuiltinTable(tableName)) + builtinOp = static_cast(pIntrinsic->Op); + DXASSERT_NOMSG(pIntrinsic != nullptr); DXASSERT_NOMSG(argTypesVector != nullptr); std::vector &argTypes = *argTypesVector; @@ -6166,7 +6180,7 @@ bool HLSLExternalSource::MatchArguments( // Catch invalid atomic dest parameters if (iArg == kAtomicDstOperandIdx && - IsAtomicOperation(static_cast(pIntrinsic->Op))) { + IsAtomicOperation(tableName, builtinOp)) { // This produces an error for bitfields that is a bit confusing because it says uint can't cast to uint if (pType.isConstant(actx) || pCallArg->getObjectKind() == OK_BitField) { badArgIdx = std::min(badArgIdx, iArg); @@ -6368,7 +6382,7 @@ bool HLSLExternalSource::MatchArguments( if (i == 0) { // [RW]ByteAddressBuffer.Load, default to uint pNewType = m_context->UnsignedIntTy; - if (pIntrinsic->Op != (UINT)hlsl::IntrinsicOp::MOP_Load) + if (builtinOp != hlsl::IntrinsicOp::MOP_Load) badArgIdx = std::min(badArgIdx, i); } else { @@ -9994,7 +10008,7 @@ Sema::TemplateDeductionResult HLSLExternalSource::DeduceTemplateArgumentsForHLSL while (cursor != end) { size_t badArgIdx; - if (!MatchArguments(*cursor, objectElement, functionTemplateTypeArg, Args, &argTypes, badArgIdx)) + if (!MatchArguments(cursor, objectElement, functionTemplateTypeArg, Args, &argTypes, badArgIdx)) { ++cursor; continue; @@ -10055,7 +10069,8 @@ Sema::TemplateDeductionResult HLSLExternalSource::DeduceTemplateArgumentsForHLSL DXASSERT_NOMSG(Specialization->getPrimaryTemplate()->getCanonicalDecl() == FunctionTemplate->getCanonicalDecl()); - if (IsBuiltinTable(tableName) && !IsValidObjectElement(*cursor, objectElement)) { + const HLSL_INTRINSIC* pIntrinsic = *cursor; + if (!IsValidObjectElement(tableName, static_cast(pIntrinsic->Op), objectElement)) { UINT numEles = GetNumElements(objectElement); std::string typeName(g_ArBasicTypeNames[GetTypeElementKind(objectElement)]); if (numEles > 1) typeName += std::to_string(numEles);