From c57989b771bd5815382c5fe8eac134d43f1b77a6 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Tue, 7 Apr 2020 13:10:26 -0400 Subject: [PATCH 1/4] Add support for new clspv interface types * Add support for kernel decls * no-op * Add support for pod args in push constants * piggybacks push constant support * pod buffer generation supports push constants * New test * Don't assert on unknown interface types in clspv * update OpenCL SET example * Refactor some code into CreatePushConstantBuffer --- src/clspv_helper.cc | 12 +- src/pipeline.cc | 212 +++++++++++++++++-------------- src/pipeline.h | 5 +- src/pipeline_test.cc | 86 +++++++++++++ tests/cases/opencl_set_arg.amber | 12 ++ 5 files changed, 228 insertions(+), 99 deletions(-) diff --git a/src/clspv_helper.cc b/src/clspv_helper.cc index 32edd0783..1db778575 100644 --- a/src/clspv_helper.cc +++ b/src/clspv_helper.cc @@ -64,6 +64,10 @@ Result Compile(Pipeline::ShaderInfo* shader_info, descriptor_entry.kind = Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_UBO; break; + case clspv::ArgKind::PodPushConstant: + descriptor_entry.kind = + Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_PUSHCONSTANT; + break; case clspv::ArgKind::ReadOnlyImage: descriptor_entry.kind = Pipeline::ShaderInfo::DescriptorMapEntry::Kind::RO_IMAGE; @@ -84,7 +88,8 @@ Result Compile(Pipeline::ShaderInfo* shader_info, } if (entry.kernel_arg_data.arg_kind == clspv::ArgKind::Pod || - entry.kernel_arg_data.arg_kind == clspv::ArgKind::PodUBO) { + entry.kernel_arg_data.arg_kind == clspv::ArgKind::PodUBO || + entry.kernel_arg_data.arg_kind == clspv::ArgKind::PodPushConstant) { descriptor_entry.pod_offset = entry.kernel_arg_data.pod_offset; descriptor_entry.pod_arg_size = entry.kernel_arg_data.pod_arg_size; } @@ -112,11 +117,12 @@ Result Compile(Pipeline::ShaderInfo* shader_info, push_constant.offset = entry.push_constant_data.offset; push_constant.size = entry.push_constant_data.size; shader_info->AddPushConstant(std::move(push_constant)); - } else { - assert(entry.kind == clspv::version0::DescriptorMapEntry::Sampler); + } else if (entry.kind == clspv::version0::DescriptorMapEntry::Sampler) { // Create a new sampler info. pipeline->AddSampler(entry.sampler_data.mask, entry.descriptor_set, entry.binding); + } else if (entry.kind == clspv::version0::DescriptorMapEntry::KernelDecl) { + // Nothing to do for declarations. } } diff --git a/src/pipeline.cc b/src/pipeline.cc index bebaf79a0..612fdbdf8 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -367,6 +367,28 @@ Result Pipeline::SetPushConstantBuffer(Buffer* buf) { return {}; } +Result Pipeline::CreatePushConstantBuffer() { + if (push_constant_buffer_.buffer != nullptr) + return Result("can only bind one push constant buffer in a PIPELINE"); + + TypeParser parser; + auto type = parser.Parse("R8_UINT"); + auto fmt = MakeUnique(type.get()); + + std::unique_ptr buf = MakeUnique(); + buf->SetName(kGeneratedPushConstantBuffer); + buf->SetFormat(fmt.get()); + + push_constant_buffer_.buffer = buf.get(); + push_constant_buffer_.type = BufferType::kPushConstant; + + formats_.push_back(std::move(fmt)); + types_.push_back(std::move(type)); + opencl_push_constants_ = std::move(buf); + + return {}; +} + std::unique_ptr Pipeline::GenerateDefaultColorAttachmentBuffer() { TypeParser parser; auto type = parser.Parse(kDefaultColorBufferFormat); @@ -664,7 +686,9 @@ Result Pipeline::GenerateOpenCLPodBuffers() { for (const auto& entry : iter->second) { if (entry.kind != Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD && entry.kind != - Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_UBO) { + Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_UBO && + entry.kind != Pipeline::ShaderInfo::DescriptorMapEntry::Kind:: + POD_PUSHCONSTANT) { continue; } @@ -680,81 +704,91 @@ Result Pipeline::GenerateOpenCLPodBuffers() { } } - if (descriptor_set == std::numeric_limits::max() || - binding == std::numeric_limits::max()) { - std::string message = - "could not find descriptor map entry for SET command: kernel " + - shader_info.GetEntryPoint(); - if (uses_name) { - message += ", name " + arg_info.name; - } else { - message += ", number " + std::to_string(arg_info.ordinal); - } - return Result(message); - } - - auto buf_iter = opencl_pod_buffer_map_.lower_bound( - std::make_pair(descriptor_set, binding)); Buffer* buffer = nullptr; - if (buf_iter == opencl_pod_buffer_map_.end() || - buf_iter->first.first != descriptor_set || - buf_iter->first.second != binding) { - // Ensure no buffer was previously bound for this descriptor set and - // binding pair. - for (const auto& buf_info : GetBuffers()) { - if (buf_info.descriptor_set == descriptor_set && - buf_info.binding == binding) { - return Result("previously bound buffer " + - buf_info.buffer->GetName() + - " to PoD args at descriptor set " + - std::to_string(descriptor_set) + " binding " + - std::to_string(binding)); - } + if (kind == + Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_PUSHCONSTANT) { + if (GetPushConstantBuffer().buffer == nullptr) { + auto r = CreatePushConstantBuffer(); + if (!r.IsSuccess()) + return r; } - - // Add a new buffer for this descriptor set and binding. - opencl_pod_buffers_.push_back(MakeUnique()); - buffer = opencl_pod_buffers_.back().get(); - auto buffer_type = - kind == Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD - ? BufferType::kStorage - : BufferType::kUniform; - - // Use an 8-bit type because all the data in the descriptor map is - // byte-based and it simplifies the logic for sizing below. - TypeParser parser; - auto type = parser.Parse("R8_UINT"); - auto fmt = MakeUnique(type.get()); - buffer->SetFormat(fmt.get()); - formats_.push_back(std::move(fmt)); - types_.push_back(std::move(type)); - - buffer->SetName(GetName() + "_pod_buffer_" + - std::to_string(descriptor_set) + "_" + - std::to_string(binding)); - opencl_pod_buffer_map_.insert( - buf_iter, - std::make_pair(std::make_pair(descriptor_set, binding), buffer)); - AddBuffer(buffer, buffer_type, descriptor_set, binding, 0); + buffer = GetPushConstantBuffer().buffer; } else { - buffer = buf_iter->second; - } + if (descriptor_set == std::numeric_limits::max() || + binding == std::numeric_limits::max()) { + std::string message = + "could not find descriptor map entry for SET command: kernel " + + shader_info.GetEntryPoint(); + if (uses_name) { + message += ", name " + arg_info.name; + } else { + message += ", number " + std::to_string(arg_info.ordinal); + } + return Result(message); + } - // Resize if necessary. - if (buffer->ValueCount() < offset + arg_size) { - buffer->SetSizeInElements(offset + arg_size); - } + auto buf_iter = opencl_pod_buffer_map_.lower_bound( + std::make_pair(descriptor_set, binding)); + if (buf_iter == opencl_pod_buffer_map_.end() || + buf_iter->first.first != descriptor_set || + buf_iter->first.second != binding) { + // Ensure no buffer was previously bound for this descriptor set and + // binding pair. + for (const auto& buf_info : GetBuffers()) { + if (buf_info.descriptor_set == descriptor_set && + buf_info.binding == binding) { + return Result("previously bound buffer " + + buf_info.buffer->GetName() + + " to PoD args at descriptor set " + + std::to_string(descriptor_set) + " binding " + + std::to_string(binding)); + } + } - // Check the data size. - if (arg_size != arg_info.fmt->SizeInBytes()) { - std::string message = "SET command uses incorrect data size: kernel " + - shader_info.GetEntryPoint(); - if (uses_name) { - message += ", name " + arg_info.name; + // Add a new buffer for this descriptor set and binding. + opencl_pod_buffers_.push_back(MakeUnique()); + buffer = opencl_pod_buffers_.back().get(); + auto buffer_type = + kind == Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD + ? BufferType::kStorage + : BufferType::kUniform; + + // Use an 8-bit type because all the data in the descriptor map is + // byte-based and it simplifies the logic for sizing below. + TypeParser parser; + auto type = parser.Parse("R8_UINT"); + auto fmt = MakeUnique(type.get()); + buffer->SetFormat(fmt.get()); + formats_.push_back(std::move(fmt)); + types_.push_back(std::move(type)); + + buffer->SetName(GetName() + "_pod_buffer_" + + std::to_string(descriptor_set) + "_" + + std::to_string(binding)); + opencl_pod_buffer_map_.insert( + buf_iter, + std::make_pair(std::make_pair(descriptor_set, binding), buffer)); + AddBuffer(buffer, buffer_type, descriptor_set, binding, 0); } else { - message += ", number " + std::to_string(arg_info.ordinal); + buffer = buf_iter->second; + } + + // Resize if necessary. + if (buffer->ValueCount() < offset + arg_size) { + buffer->SetSizeInElements(offset + arg_size); + } + + // Check the data size. + if (arg_size != arg_info.fmt->SizeInBytes()) { + std::string message = "SET command uses incorrect data size: kernel " + + shader_info.GetEntryPoint(); + if (uses_name) { + message += ", name " + arg_info.name; + } else { + message += ", number " + std::to_string(arg_info.ordinal); + } + return Result(message); } - return Result(message); } // Convert the argument value into bytes. Currently, only scalar arguments @@ -864,17 +898,23 @@ Result Pipeline::GenerateOpenCLPushConstants() { if (shader_info.GetPushConstants().empty()) return {}; + Result r = CreatePushConstantBuffer(); + if (!r.IsSuccess()) + return r; + + auto* buf = GetPushConstantBuffer().buffer; + assert(buf); + // Determine size and contents of the push constant buffer. - std::vector bytes; for (const auto& pc : shader_info.GetPushConstants()) { assert(pc.size % sizeof(uint32_t) == 0); assert(pc.offset % sizeof(uint32_t) == 0); - uint32_t elements = (pc.offset + pc.size) / sizeof(uint32_t); - if (elements > bytes.size()) { - bytes.resize(elements); - } - uint32_t base = pc.offset / sizeof(uint32_t); + if (buf->GetSizeInBytes() < pc.offset + pc.size) + buf->SetSizeInBytes(pc.offset + pc.size); + + std::vector bytes(pc.size / sizeof(uint32_t)); + uint32_t base = 0; switch (pc.type) { case Pipeline::ShaderInfo::PushConstant::PushConstantType::kDimensions: // All compute kernel launches are 3D. @@ -887,28 +927,10 @@ Result Pipeline::GenerateOpenCLPushConstants() { bytes[base + 2] = 0; break; } + memcpy(buf->ValuePtr()->data() + pc.offset, bytes.data(), + bytes.size() * sizeof(uint32_t)); } - TypeParser parser; - auto type = parser.Parse("R8_UINT"); - auto fmt = MakeUnique(type.get()); - - // Create buffer containing push constant data. - std::unique_ptr buf = MakeUnique(); - buf->SetName(kGeneratedPushConstantBuffer); - buf->SetFormat(fmt.get()); - buf->SetSizeInBytes(static_cast(bytes.size() * sizeof(uint32_t))); - std::memcpy(buf->ValuePtr()->data(), bytes.data(), - bytes.size() * sizeof(uint32_t)); - - Result r = SetPushConstantBuffer(buf.get()); - if (!r.IsSuccess()) - return r; - - formats_.push_back(std::move(fmt)); - types_.push_back(std::move(type)); - opencl_push_constants_ = std::move(buf); - return {}; } diff --git a/src/pipeline.h b/src/pipeline.h index b565eca62..8bd786052 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -91,6 +91,7 @@ class Pipeline { UBO, POD, POD_UBO, + POD_PUSHCONSTANT, RO_IMAGE, WO_IMAGE, SAMPLER, @@ -116,7 +117,7 @@ class Pipeline { struct PushConstant { enum class PushConstantType { kDimensions = 0, - kGlobalOffset, + kGlobalOffset }; PushConstantType type; uint32_t offset = 0; @@ -344,6 +345,8 @@ class Pipeline { private: void UpdateFramebufferSizes(); + Result CreatePushConstantBuffer(); + Result ValidateGraphics() const; Result ValidateCompute() const; diff --git a/src/pipeline_test.cc b/src/pipeline_test.cc index de7736996..e0084cee9 100644 --- a/src/pipeline_test.cc +++ b/src/pipeline_test.cc @@ -856,4 +856,90 @@ TEST_F(PipelineTest, OpenCLGeneratePushConstants) { EXPECT_EQ(0U, bytes[6]); } +TEST_F(PipelineTest, OpenCLPodPushConstants) { + Pipeline p(PipelineType::kCompute); + p.SetName("my_pipeline"); + + Shader cs(kShaderTypeCompute); + cs.SetFormat(kShaderFormatOpenCLC); + p.AddShader(&cs, kShaderTypeCompute); + p.SetShaderEntryPoint(&cs, "my_main"); + + // Descriptor map. + Pipeline::ShaderInfo::DescriptorMapEntry entry1; + entry1.kind = + Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_PUSHCONSTANT; + entry1.descriptor_set = -1; + entry1.binding = -1; + entry1.arg_name = "arg_a"; + entry1.arg_ordinal = 0; + entry1.pod_offset = 0; + entry1.pod_arg_size = 4; + p.GetShaders()[0].AddDescriptorEntry("my_main", std::move(entry1)); + + Pipeline::ShaderInfo::DescriptorMapEntry entry2; + entry2.kind = + Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_PUSHCONSTANT; + entry2.descriptor_set = -1; + entry2.binding = -1; + entry2.arg_name = "arg_b"; + entry2.arg_ordinal = 1; + entry2.pod_offset = 4; + entry2.pod_arg_size = 1; + p.GetShaders()[0].AddDescriptorEntry("my_main", std::move(entry2)); + + Pipeline::ShaderInfo::DescriptorMapEntry entry3; + entry3.kind = + Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_PUSHCONSTANT; + entry3.descriptor_set = -1; + entry3.binding = -1; + entry3.arg_name = "arg_c"; + entry3.arg_ordinal = 2; + entry3.pod_offset = 8; + entry3.pod_arg_size = 4; + p.GetShaders()[0].AddDescriptorEntry("my_main", std::move(entry3)); + + // Set commands. + Value int_value; + int_value.SetIntValue(1); + + TypeParser parser; + auto int_type = parser.Parse("R32_SINT"); + auto int_fmt = MakeUnique(int_type.get()); + auto char_type = parser.Parse("R8_SINT"); + auto char_fmt = MakeUnique(char_type.get()); + + Pipeline::ArgSetInfo arg_info1; + arg_info1.name = "arg_a"; + arg_info1.ordinal = 99; + arg_info1.fmt = int_fmt.get(); + arg_info1.value = int_value; + p.SetArg(std::move(arg_info1)); + + Pipeline::ArgSetInfo arg_info2; + arg_info2.name = "arg_b"; + arg_info2.ordinal = 99; + arg_info2.fmt = char_fmt.get(); + arg_info2.value = int_value; + p.SetArg(std::move(arg_info2)); + + Pipeline::ArgSetInfo arg_info3; + arg_info3.name = "arg_c"; + arg_info3.ordinal = 99; + arg_info3.fmt = int_fmt.get(); + arg_info3.value = int_value; + p.SetArg(std::move(arg_info3)); + + auto r = p.GenerateOpenCLPodBuffers(); + auto* buf = p.GetPushConstantBuffer().buffer; + EXPECT_NE(nullptr, buf); + EXPECT_EQ(12U, buf->GetSizeInBytes()); + + const uint32_t* ints = buf->GetValues(); + const uint8_t* bytes = buf->GetValues(); + EXPECT_EQ(1U, ints[0]); + EXPECT_EQ(1U, bytes[4]); + EXPECT_EQ(1U, ints[2]); +} + } // namespace amber diff --git a/tests/cases/opencl_set_arg.amber b/tests/cases/opencl_set_arg.amber index eb3358e9f..b73e8bef0 100644 --- a/tests/cases/opencl_set_arg.amber +++ b/tests/cases/opencl_set_arg.amber @@ -24,6 +24,7 @@ BUFFER out_buf1 DATA_TYPE uint32 DATA 0 END BUFFER out_buf2 DATA_TYPE uint32 DATA 0 END BUFFER out_buf3 DATA_TYPE uint32 DATA 0 END BUFFER out_buf4 DATA_TYPE uint32 DATA 0 END +BUFFER out_buf5 DATA_TYPE uint32 DATA 0 END PIPELINE compute p1 ATTACH my_shader ENTRY_POINT line @@ -56,12 +57,23 @@ DERIVE_PIPELINE p4 FROM p1 END END +DERIVE_PIPELINE p5 FROM p1 + BIND BUFFER out_buf5 KERNEL ARG_NAME out + SET KERNEL ARG_NAME slope AS int32 3 + COMPILE_OPTIONS my_shader + -cluster-pod-kernel-args + -pod-pushconstant + END +END + RUN p1 1 1 1 RUN p2 1 1 1 RUN p3 1 1 1 RUN p4 1 1 1 +RUN p5 1 1 1 EXPECT out_buf1 IDX 0 EQ 7 EXPECT out_buf2 IDX 0 EQ 7 EXPECT out_buf3 IDX 0 EQ 7 EXPECT out_buf4 IDX 0 EQ 10 +EXPECT out_buf5 IDX 0 EQ 10 From ca3c64f589acc90205d1c2bbc6792d4e29494472 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Tue, 7 Apr 2020 13:14:39 -0400 Subject: [PATCH 2/4] Update clspv deps --- DEPS | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/DEPS b/DEPS index 507242ab0..5c37440d4 100644 --- a/DEPS +++ b/DEPS @@ -9,8 +9,8 @@ vars = { 'nlohmann_git': 'https://github.com/nlohmann', 'swiftshader_git': 'https://swiftshader.googlesource.com', - 'clspv_llvm_revision': '627e01feb718dd1aa8e184545976b7229de312a2', - 'clspv_revision': '1356838737b0fbe1f3b9d0744df46ebc716d7b5c', + 'clspv_llvm_revision': 'a2bb19ca420d0801f5b9a5363dec1d7bcb829030', + 'clspv_revision': '4e6c283e15420ba6e949d44850929b8d78b45ad9', 'cppdap_revision': '4dcca5775616ada2796ff7f84c3a4843eee9b506', 'cpplint_revision': '26470f9ccb354ff2f6d098f831271a1833701b28', 'dxc_revision': 'ef73f1da8ad79a42405e8c75cf7d66ed8a0bb345', From 4995a166656e36568416aa95eb96d623911570e1 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Tue, 7 Apr 2020 13:27:48 -0400 Subject: [PATCH 3/4] Revert uninitentional change --- src/pipeline.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pipeline.h b/src/pipeline.h index 8bd786052..36c87aaf2 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -117,7 +117,7 @@ class Pipeline { struct PushConstant { enum class PushConstantType { kDimensions = 0, - kGlobalOffset + kGlobalOffset, }; PushConstantType type; uint32_t offset = 0; From 5a2e6f0368a090560db07628b8710e5169a1cee5 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Tue, 7 Apr 2020 14:37:05 -0400 Subject: [PATCH 4/4] fix build --- src/pipeline_test.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/pipeline_test.cc b/src/pipeline_test.cc index e0084cee9..160d95143 100644 --- a/src/pipeline_test.cc +++ b/src/pipeline_test.cc @@ -869,8 +869,8 @@ TEST_F(PipelineTest, OpenCLPodPushConstants) { Pipeline::ShaderInfo::DescriptorMapEntry entry1; entry1.kind = Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_PUSHCONSTANT; - entry1.descriptor_set = -1; - entry1.binding = -1; + entry1.descriptor_set = static_cast(-1); + entry1.binding = static_cast(-1); entry1.arg_name = "arg_a"; entry1.arg_ordinal = 0; entry1.pod_offset = 0; @@ -880,8 +880,8 @@ TEST_F(PipelineTest, OpenCLPodPushConstants) { Pipeline::ShaderInfo::DescriptorMapEntry entry2; entry2.kind = Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_PUSHCONSTANT; - entry2.descriptor_set = -1; - entry2.binding = -1; + entry2.descriptor_set = static_cast(-1); + entry2.binding = static_cast(-1); entry2.arg_name = "arg_b"; entry2.arg_ordinal = 1; entry2.pod_offset = 4; @@ -891,8 +891,8 @@ TEST_F(PipelineTest, OpenCLPodPushConstants) { Pipeline::ShaderInfo::DescriptorMapEntry entry3; entry3.kind = Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_PUSHCONSTANT; - entry3.descriptor_set = -1; - entry3.binding = -1; + entry3.descriptor_set = static_cast(-1); + entry3.binding = static_cast(-1); entry3.arg_name = "arg_c"; entry3.arg_ordinal = 2; entry3.pod_offset = 8;