diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md index a9232d4e29..065e19667d 100644 --- a/docs/ReleaseNotes.md +++ b/docs/ReleaseNotes.md @@ -34,6 +34,13 @@ line upon naming the release. Refer to previous for appropriate section names. instead of crashing [#6661](https://github.com/microsoft/DirectXShaderCompiler/issues/6661). +#### SPIR-V + +- Fixed a crash from `static` members of a `cbuffer`/`tbuffer`, which were + incorrectly counted as buffer members and shifted the member indices of the + members declared after them + [#8537](https://github.com/microsoft/DirectXShaderCompiler/issues/8537). + #### Bug Fixes - Fixed internal compiler errors when a member method is called on a ray payload diff --git a/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp b/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp index 3a88334ffc..c6affae44f 100644 --- a/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp +++ b/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp @@ -299,6 +299,14 @@ LocationAndComponent getLocationAndComponentCount(const ASTContext &astContext, return {0, 0, false}; } +/// Returns true if the given decl is a static variable declared inside +/// a cbuffer/tbuffer/struct. These are not actually part of the +/// cbuffer/tbuffer/struct, and should not consume a member index. +bool isStaticBufferDecl(const Decl *decl) { + const auto *varDecl = dyn_cast(decl); + return varDecl && varDecl->getStorageClass() == StorageClass::SC_Static; +} + bool shouldSkipInStructLayout(const Decl *decl) { // Ignore implicit generated struct declarations/constructors/destructors if (decl->isImplicit()) @@ -1455,7 +1463,7 @@ SpirvVariable *DeclResultIdMapper::createStructOrStructArrayVarOfExplicitLayout( // Static variables are not part of the struct from a layout perspective. // Thus, they should not be listed in the struct fields. - if (fieldVar->getStorageClass() == StorageClass::SC_Static) { + if (isStaticBufferDecl(fieldVar)) { continue; } @@ -1558,8 +1566,11 @@ void DeclResultIdMapper::createCTBuffer(const HLSLBufferDecl *decl) { if (shouldSkipInStructLayout(subDecl)) continue; - // If the subDecl is a resource, it is lowered as a standalone variable. const auto *varDecl = cast(subDecl); + + if (isStaticBufferDecl(varDecl)) + continue; + if (isResourceType(varDecl->getType())) { createExternVar(varDecl); continue; @@ -1737,6 +1748,9 @@ DeclResultIdMapper::createShaderRecordBuffer(const HLSLBufferDecl *decl, if (isResourceType(varDecl->getType())) continue; + if (isStaticBufferDecl(varDecl)) + continue; + registerVariableForDecl(varDecl, createDeclSpirvInfo(bufferVar, index++)); } return bufferVar; diff --git a/tools/clang/lib/SPIRV/SpirvEmitter.cpp b/tools/clang/lib/SPIRV/SpirvEmitter.cpp index f05c0d9553..71f0771d9e 100644 --- a/tools/clang/lib/SPIRV/SpirvEmitter.cpp +++ b/tools/clang/lib/SPIRV/SpirvEmitter.cpp @@ -1941,6 +1941,9 @@ void SpirvEmitter::doHLSLBufferDecl(const HLSLBufferDecl *bufferDecl) { // supported in Vulkan for (const auto *member : bufferDecl->decls()) { if (const auto *varMember = dyn_cast(member)) { + if (varMember->getStorageClass() == StorageClass::SC_Static) + continue; + if (!spirvOptions.noWarnIgnoredFeatures) { if (const auto *init = varMember->getInit()) emitWarning("%select{tbuffer|cbuffer}0 member initializer " @@ -1970,6 +1973,12 @@ void SpirvEmitter::doHLSLBufferDecl(const HLSLBufferDecl *bufferDecl) { } else { declIdMapper.createCTBuffer(bufferDecl); } + + for (const auto *member : bufferDecl->decls()) { + const auto *varMember = dyn_cast(member); + if (varMember && varMember->getStorageClass() == StorageClass::SC_Static) + doVarDecl(varMember); + } } void SpirvEmitter::doClassTemplateDecl( @@ -2165,9 +2174,11 @@ void SpirvEmitter::doVarDecl(const VarDecl *decl) { // ConstantBuffers and TextureBuffers are not HLSLBufferDecls. if (const auto *bufferDecl = dyn_cast(decl->getDeclContext())) { - // This is a VarDecl of cbuffer/tbuffer type. - doHLSLBufferDecl(bufferDecl); - return; + if (decl->getStorageClass() != StorageClass::SC_Static) { + // This is a VarDecl of cbuffer/tbuffer type. + doHLSLBufferDecl(bufferDecl); + return; + } } if (decl->getAttr()) { diff --git a/tools/clang/test/CodeGenSPIRV/cbuffer.static.member.hlsl b/tools/clang/test/CodeGenSPIRV/cbuffer.static.member.hlsl new file mode 100644 index 0000000000..3f5884e905 --- /dev/null +++ b/tools/clang/test/CodeGenSPIRV/cbuffer.static.member.hlsl @@ -0,0 +1,34 @@ +// RUN: %dxc -T vs_6_0 -E main -fcgl %s -spirv | FileCheck %s + +// A `static` variable declared inside a cbuffer is an ordinary global variable, +// not a member of the buffer. It must be left out of the buffer's struct (it is +// not part of the layout) *and* it must not consume a member index, otherwise +// the access chains for the members declared after it are shifted, and the last +// one ends up out of bounds. + +// CHECK: OpMemberName %type_MyCBuffer 0 "a" +// CHECK: OpMemberName %type_MyCBuffer 1 "b" +// CHECK: OpMemberName %type_MyCBuffer 2 "c" +// CHECK-NOT: OpMemberName %type_MyCBuffer 3 + +// CHECK-DAG: %uint_10 = OpConstant %uint 10 +// CHECK-DAG: %uint_11 = OpConstant %uint 11 +// CHECK-DAG: %uint_12 = OpConstant %uint 12 + +// CHECK: %type_MyCBuffer = OpTypeStruct %uint %uint %uint + +cbuffer MyCBuffer { + uint a; + static const uint a_mode = 10; + uint b; + static const uint b_mode = 11; + uint c; + static const uint c_mode = 12; +}; + +float4 main() : SV_Position { +// CHECK: OpAccessChain %_ptr_Uniform_uint %MyCBuffer %int_0 +// CHECK: OpAccessChain %_ptr_Uniform_uint %MyCBuffer %int_1 +// CHECK: OpAccessChain %_ptr_Uniform_uint %MyCBuffer %int_2 + return float4(a + b + c, a_mode, b_mode, c_mode); +} diff --git a/tools/clang/test/CodeGenSPIRV/vk.shader-record-ext.static.member.hlsl b/tools/clang/test/CodeGenSPIRV/vk.shader-record-ext.static.member.hlsl new file mode 100644 index 0000000000..44e9a05798 --- /dev/null +++ b/tools/clang/test/CodeGenSPIRV/vk.shader-record-ext.static.member.hlsl @@ -0,0 +1,41 @@ +// RUN: %dxc -T lib_6_3 -fspv-target-env=vulkan1.2 -fcgl %s -spirv | FileCheck %s + +// Same as cbuffer.static.member.hlsl, but for a shader record buffer, which +// goes through createShaderRecordBuffer() instead of createCTBuffer(). A +// `static` variable declared inside the buffer is an ordinary global variable, +// not a member of the buffer. It must be left out of the buffer's struct (it is +// not part of the layout) *and* it must not consume a member index, otherwise +// the access chains for the members declared after it are shifted, and the last +// one ends up out of bounds. + +// CHECK: OpMemberName %type_ShaderRecordBufferKHR_block 0 "a" +// CHECK: OpMemberName %type_ShaderRecordBufferKHR_block 1 "b" +// CHECK: OpMemberName %type_ShaderRecordBufferKHR_block 2 "c" +// CHECK-NOT: OpMemberName %type_ShaderRecordBufferKHR_block 3 + +// CHECK-DAG: %uint_10 = OpConstant %uint 10 +// CHECK-DAG: %uint_11 = OpConstant %uint 11 +// CHECK-DAG: %uint_12 = OpConstant %uint 12 + +// CHECK: %type_ShaderRecordBufferKHR_block = OpTypeStruct %uint %uint %uint + +[[vk::shader_record_ext]] +cbuffer block { + uint a; + static const uint a_mode = 10; + uint b; + static const uint b_mode = 11; + uint c; + static const uint c_mode = 12; +} + +struct Payload { float p; }; +struct Attr { float a; }; + +[shader("closesthit")] +void main(inout Payload P, in Attr A) { +// CHECK: OpAccessChain %_ptr_ShaderRecordBufferKHR_uint %block %int_0 +// CHECK: OpAccessChain %_ptr_ShaderRecordBufferKHR_uint %block %int_1 +// CHECK: OpAccessChain %_ptr_ShaderRecordBufferKHR_uint %block %int_2 + P.p = a + b + c + a_mode + b_mode + c_mode; +}