From 6ac1f631135f47c452c4765836504880ced7944a Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Tue, 14 Jul 2020 22:55:34 -0400 Subject: [PATCH 1/3] [spirv] Remove BufferBlock for array types --- .../lib/SPIRV/RemoveBufferBlockVisitor.cpp | 26 ++++++++++++++----- .../lib/SPIRV/RemoveBufferBlockVisitor.h | 5 ++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp b/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp index 1819b3f6a4..fd3210a1e4 100644 --- a/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp +++ b/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp @@ -33,6 +33,23 @@ bool RemoveBufferBlockVisitor::visit(SpirvModule *mod, Phase phase) { return true; } +bool RemoveBufferBlockVisitor::HasStorageBufferInterfaceType( + const SpirvType *type) { + while (type != nullptr) { + if (const auto *structType = dyn_cast(type)) { + return structType->getInterfaceType() == + StructInterfaceType::StorageBuffer; + } else if (const auto *elemType = dyn_cast(type)) { + type = elemType->getElementType(); + } else if (const auto *elemType = dyn_cast(type)) { + type = elemType->getElementType(); + } else { + return false; + } + } + return false; +} + bool RemoveBufferBlockVisitor::visitInstruction(SpirvInstruction *inst) { if (!inst->getResultType()) return true; @@ -60,13 +77,10 @@ bool RemoveBufferBlockVisitor::visitInstruction(SpirvInstruction *inst) { // For all instructions, if the result type is a pointer pointing to a struct // with StorageBuffer interface, the storage class must be updated. if (auto *ptrResultType = dyn_cast(inst->getResultType())) { - if (auto *structPointeeType = - dyn_cast(ptrResultType->getPointeeType())) { + if (HasStorageBufferInterfaceType(ptrResultType->getPointeeType())) { // Update the instruction's storage class if necessary - if (structPointeeType->getInterfaceType() == - StructInterfaceType::StorageBuffer && - ptrResultType->getStorageClass() != - spv::StorageClass::StorageBuffer) { + if (ptrResultType->getStorageClass() != + spv::StorageClass::StorageBuffer) { inst->setStorageClass(spv::StorageClass::StorageBuffer); inst->setResultType(context.getPointerType( ptrResultType->getPointeeType(), spv::StorageClass::StorageBuffer)); diff --git a/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h b/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h index 61c2e30d57..0dbd1d3d19 100644 --- a/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h +++ b/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h @@ -33,6 +33,11 @@ class RemoveBufferBlockVisitor : public Visitor { /// So that you want override this visit function to handle all instructions, /// regardless of their polymorphism. bool visitInstruction(SpirvInstruction *instr) override; + +private: + /// Returns true if |type| is a SPIR-V type whose interface type is + /// StorageBuffer. + bool HasStorageBufferInterfaceType(const SpirvType *type); }; } // end namespace spirv From 903d1c3b57c342cbddfd1f0110decb8f41ba9a04 Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Tue, 14 Jul 2020 23:18:39 -0400 Subject: [PATCH 2/3] Add a unit test --- ...k.1p2.remove.bufferblock.runtimearray.hlsl | 26 +++++++++++++++++++ .../unittests/SPIRV/CodeGenSpirvTest.cpp | 4 +++ 2 files changed, 30 insertions(+) create mode 100644 tools/clang/test/CodeGenSPIRV/vk.1p2.remove.bufferblock.runtimearray.hlsl diff --git a/tools/clang/test/CodeGenSPIRV/vk.1p2.remove.bufferblock.runtimearray.hlsl b/tools/clang/test/CodeGenSPIRV/vk.1p2.remove.bufferblock.runtimearray.hlsl new file mode 100644 index 0000000000..aa17b3277d --- /dev/null +++ b/tools/clang/test/CodeGenSPIRV/vk.1p2.remove.bufferblock.runtimearray.hlsl @@ -0,0 +1,26 @@ +// Run: %dxc -T cs_6_6 -E main -fspv-target-env=vulkan1.2 + +struct MeshPart { + uint indexOffset; + uint positionOffset; + uint normalOffset; + uint texCoord0Offset; +}; + +// CHECK: %_ptr_StorageBuffer_type_StructuredBuffer_MeshPart = OpTypePointer StorageBuffer %type_StructuredBuffer_MeshPart +// CHECK: %_ptr_StorageBuffer_uint = OpTypePointer StorageBuffer %uint + +StructuredBuffer g_meshParts[] : register(t2, space1); +RWStructuredBuffer g_output : register(u1, space2); + +// CHECK: %g_meshParts = OpVariable %_ptr_StorageBuffer__runtimearr_type_StructuredBuffer_MeshPart StorageBuffer +// CHECK: %g_output = OpVariable %_ptr_StorageBuffer_type_RWStructuredBuffer_uint StorageBuffer + +[numthreads(64, 1, 1)] +void main() +{ + MeshPart meshPart = g_meshParts[0][0]; + g_output[0] = meshPart.indexOffset; +// CHECK: OpAccessChain %_ptr_StorageBuffer_type_StructuredBuffer_MeshPart %g_meshParts %int_0 +// CHECK: OpAccessChain %_ptr_StorageBuffer_uint %g_output %int_0 %uint_0 +} diff --git a/tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp b/tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp index 322c5b8dc1..72b8023cfb 100644 --- a/tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp +++ b/tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp @@ -2336,6 +2336,10 @@ TEST_F(FileTest, Vk1p2BlockDecoration) { useVulkan1p2(); runFileTest("vk.1p2.block-decoration.hlsl"); } +TEST_F(FileTest, Vk1p2RemoveBufferBlockRuntimeArray) { + useVulkan1p2(); + runFileTest("vk.1p2.remove.bufferblock.runtimearray.hlsl"); +} // Test shaders that require Vulkan1.1 support with // -fspv-target-env=vulkan1.2 option to make sure that enabling From 723d05722ba38a814855d479cbb678854407ea76 Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Wed, 15 Jul 2020 10:40:46 -0400 Subject: [PATCH 3/3] code review --- .../clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp | 15 ++++++--------- tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h | 2 +- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp b/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp index fd3210a1e4..91c91df850 100644 --- a/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp +++ b/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.cpp @@ -33,7 +33,7 @@ bool RemoveBufferBlockVisitor::visit(SpirvModule *mod, Phase phase) { return true; } -bool RemoveBufferBlockVisitor::HasStorageBufferInterfaceType( +bool RemoveBufferBlockVisitor::hasStorageBufferInterfaceType( const SpirvType *type) { while (type != nullptr) { if (const auto *structType = dyn_cast(type)) { @@ -77,14 +77,11 @@ bool RemoveBufferBlockVisitor::visitInstruction(SpirvInstruction *inst) { // For all instructions, if the result type is a pointer pointing to a struct // with StorageBuffer interface, the storage class must be updated. if (auto *ptrResultType = dyn_cast(inst->getResultType())) { - if (HasStorageBufferInterfaceType(ptrResultType->getPointeeType())) { - // Update the instruction's storage class if necessary - if (ptrResultType->getStorageClass() != - spv::StorageClass::StorageBuffer) { - inst->setStorageClass(spv::StorageClass::StorageBuffer); - inst->setResultType(context.getPointerType( - ptrResultType->getPointeeType(), spv::StorageClass::StorageBuffer)); - } + if (hasStorageBufferInterfaceType(ptrResultType->getPointeeType()) && + ptrResultType->getStorageClass() != spv::StorageClass::StorageBuffer) { + inst->setStorageClass(spv::StorageClass::StorageBuffer); + inst->setResultType(context.getPointerType( + ptrResultType->getPointeeType(), spv::StorageClass::StorageBuffer)); } } diff --git a/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h b/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h index 0dbd1d3d19..0e078cf040 100644 --- a/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h +++ b/tools/clang/lib/SPIRV/RemoveBufferBlockVisitor.h @@ -37,7 +37,7 @@ class RemoveBufferBlockVisitor : public Visitor { private: /// Returns true if |type| is a SPIR-V type whose interface type is /// StorageBuffer. - bool HasStorageBufferInterfaceType(const SpirvType *type); + bool hasStorageBufferInterfaceType(const SpirvType *type); }; } // end namespace spirv