From 5f88a91a43b48e7201d33e5c4a0a86f89097424b Mon Sep 17 00:00:00 2001 From: James Price Date: Wed, 11 Mar 2020 21:51:07 -0400 Subject: [PATCH 1/2] Add support for push constants generated by Clspv When the -work-dim or -global-offset flags are passed to Clspv, it will generate push constants. The sizes and offsets of the push constant values are provided in the descriptor map. Amber needs to add these to the pipeline layout and create a buffer for them. --- src/clspv_helper.cc | 18 +++++ src/executor.cc | 3 + src/pipeline.cc | 58 ++++++++++++++ src/pipeline.h | 23 ++++++ src/pipeline_test.cc | 34 ++++++++ .../opencl_generated_push_constants.amber | 79 +++++++++++++++++++ tests/run_tests.py | 1 + 7 files changed, 216 insertions(+) create mode 100644 tests/cases/opencl_generated_push_constants.amber diff --git a/src/clspv_helper.cc b/src/clspv_helper.cc index 3dbb69695..b429a7d1b 100644 --- a/src/clspv_helper.cc +++ b/src/clspv_helper.cc @@ -94,6 +94,24 @@ Result Compile(Pipeline::ShaderInfo* shader_info, shader_info->AddDescriptorEntry(entry.kernel_arg_data.kernel_name, std::move(descriptor_entry)); + } else if (entry.kind == + clspv::version0::DescriptorMapEntry::PushConstant) { + Pipeline::ShaderInfo::PushConstant push_constant; + switch (entry.push_constant_data.pc) { + case clspv::PushConstant::Dimensions: + push_constant.type = + Pipeline::ShaderInfo::PushConstant::kPushConstantDimensions; + break; + case clspv::PushConstant::GlobalOffset: + push_constant.type = + Pipeline::ShaderInfo::PushConstant::kPushConstantGlobalOffset; + break; + default: + return Result("unhandled push constant generated by Clspv"); + } + 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); // Create a new sampler info. diff --git a/src/executor.cc b/src/executor.cc index df204d67e..0d428e949 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -71,6 +71,9 @@ Result Executor::Execute(Engine* engine, r = pipeline->GenerateOpenCLLiteralSamplers(); if (!r.IsSuccess()) return r; + r = pipeline->GenerateOpenCLPushConstants(); + if (!r.IsSuccess()) + return r; } for (auto& pipeline : script->GetPipelines()) { diff --git a/src/pipeline.cc b/src/pipeline.cc index eca6326b7..951246bd9 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -45,6 +45,7 @@ const uint32_t kOpenCLFilterModeLinearBit = 0x20; const char* Pipeline::kGeneratedColorBuffer = "framebuffer"; const char* Pipeline::kGeneratedDepthBuffer = "depth_buffer"; +const char* Pipeline::kGeneratedPushConstantBuffer = "push_constant_buffer"; Pipeline::ShaderInfo::ShaderInfo(Shader* shader, ShaderType type) : shader_(shader), shader_type_(type), entry_point_("main") {} @@ -825,4 +826,61 @@ Result Pipeline::GenerateOpenCLLiteralSamplers() { return {}; } +Result Pipeline::GenerateOpenCLPushConstants() { + if (!IsCompute() || GetShaders().empty() || + GetShaders()[0].GetShader()->GetFormat() != kShaderFormatOpenCLC) { + return {}; + } + + const auto& shader_info = GetShaders()[0]; + if (shader_info.GetPushConstants().empty()) + return {}; + + // Determine size and contents of the push constant buffer. + std::vector bytes; + for (const auto& pc : shader_info.GetPushConstants()) { + if (pc.offset + pc.size > bytes.size()) { + bytes.resize(pc.offset + pc.size); + } + + switch (pc.type) { + case Pipeline::ShaderInfo::PushConstant::kPushConstantDimensions: + // All compute kernel launches are 3D. + assert(pc.size == 4); + *reinterpret_cast(bytes.data() + pc.offset) = 3; + break; + case Pipeline::ShaderInfo::PushConstant::kPushConstantGlobalOffset: { + // Global offsets are not currently supported. + assert(pc.size == 12); + uint32_t* ptr = reinterpret_cast(bytes.data() + pc.offset); + ptr[0] = 0; + ptr[1] = 0; + ptr[2] = 0; + break; + } + } + } + + 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(bytes.size()); + buf->ValuePtr()->assign(bytes.begin(), bytes.end()); + + 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 {}; +} + } // namespace amber diff --git a/src/pipeline.h b/src/pipeline.h index a25b18435..7cf595047 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -112,6 +112,23 @@ class Pipeline { return descriptor_map_; } + /// Push constant information for an OpenCL-C shader. + struct PushConstant { + enum { + kPushConstantDimensions, + kPushConstantGlobalOffset, + } type; + uint32_t offset = 0; + uint32_t size = 0; + }; + + void AddPushConstant(PushConstant&& pc) { + push_constants_.emplace_back(std::move(pc)); + } + const std::vector& GetPushConstants() const { + return push_constants_; + } + private: Shader* shader_ = nullptr; ShaderType shader_type_; @@ -121,6 +138,7 @@ class Pipeline { std::map specialization_; std::unordered_map> descriptor_map_; + std::vector push_constants_; std::vector compile_options_; }; @@ -157,6 +175,7 @@ class Pipeline { static const char* kGeneratedColorBuffer; static const char* kGeneratedDepthBuffer; + static const char* kGeneratedPushConstantBuffer; explicit Pipeline(PipelineType type); ~Pipeline(); @@ -318,6 +337,9 @@ class Pipeline { /// descriptor map. This should be called after all other samplers are bound. Result GenerateOpenCLLiteralSamplers(); + /// Generate the push constant buffers necessary for OpenCL kernels. + Result GenerateOpenCLPushConstants(); + private: void UpdateFramebufferSizes(); @@ -346,6 +368,7 @@ class Pipeline { /// Maps (descriptor set, binding) to the buffer for that binding pair. std::map, Buffer*> opencl_pod_buffer_map_; std::vector> opencl_literal_samplers_; + std::unique_ptr opencl_push_constants_; }; } // namespace amber diff --git a/src/pipeline_test.cc b/src/pipeline_test.cc index 7a2b2fedb..513346c06 100644 --- a/src/pipeline_test.cc +++ b/src/pipeline_test.cc @@ -821,4 +821,38 @@ TEST_F(PipelineTest, OpenCLGenerateLiteralSamplers) { } } +TEST_F(PipelineTest, OpenCLGeneratePushConstants) { + Pipeline p(PipelineType::kCompute); + p.SetName("my_pipeline"); + + Shader cs(kShaderTypeCompute); + cs.SetFormat(kShaderFormatOpenCLC); + p.AddShader(&cs, kShaderTypeCompute); + p.SetShaderEntryPoint(&cs, "my_main"); + + Pipeline::ShaderInfo::PushConstant pc1; + pc1.type = Pipeline::ShaderInfo::PushConstant::kPushConstantDimensions; + pc1.offset = 0; + pc1.size = 4; + p.GetShaders()[0].AddPushConstant(std::move(pc1)); + + Pipeline::ShaderInfo::PushConstant pc2; + pc2.type = Pipeline::ShaderInfo::PushConstant::kPushConstantGlobalOffset; + pc2.offset = 16; + pc2.size = 12; + p.GetShaders()[0].AddPushConstant(std::move(pc2)); + + auto r = p.GenerateOpenCLPushConstants(); + ASSERT_TRUE(r.IsSuccess()); + + const auto& buf = p.GetPushConstantBuffer(); + EXPECT_EQ(28U, buf.buffer->GetSizeInBytes()); + + uint32_t* bytes = reinterpret_cast(buf.buffer->ValuePtr()->data()); + EXPECT_EQ(3U, bytes[0]); + EXPECT_EQ(0U, bytes[4]); + EXPECT_EQ(0U, bytes[5]); + EXPECT_EQ(0U, bytes[6]); +} + } // namespace amber diff --git a/tests/cases/opencl_generated_push_constants.amber b/tests/cases/opencl_generated_push_constants.amber new file mode 100644 index 000000000..e479f5cef --- /dev/null +++ b/tests/cases/opencl_generated_push_constants.amber @@ -0,0 +1,79 @@ +#!amber +# Copyright 2020 The Amber Authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +SHADER compute work_dim_shader OPENCL-C +kernel void foo(global int* out) { + *out = get_work_dim(); +} +END + +SHADER compute global_offset_shader OPENCL-C +kernel void foo(global int* out) { + out[0] = get_global_offset(0); + out[1] = get_global_offset(1); + out[2] = get_global_offset(2); +} +END + +SHADER compute all_constants_shader OPENCL-C +kernel void foo(global int* out) { + out[0] = get_global_offset(0); + out[1] = get_global_offset(1); + out[2] = get_global_offset(2); + out[3] = get_work_dim(); +} +END + +BUFFER work_dim_buf DATA_TYPE uint32 SIZE 1 FILL -1 +BUFFER global_offset_buf DATA_TYPE uint32 SIZE 3 FILL -1 +BUFFER all_constants_buf DATA_TYPE uint32 SIZE 4 FILL -1 + +PIPELINE compute work_dim_pipeline + ATTACH work_dim_shader ENTRY_POINT foo + COMPILE_OPTIONS work_dim_shader + -work-dim + END + BIND BUFFER work_dim_buf AS storage DESCRIPTOR_SET 0 BINDING 0 +END + +PIPELINE compute global_offset_pipeline + ATTACH global_offset_shader ENTRY_POINT foo + COMPILE_OPTIONS global_offset_shader + -global-offset + END + BIND BUFFER global_offset_buf AS storage DESCRIPTOR_SET 0 BINDING 0 +END + +PIPELINE compute all_constants_pipeline + ATTACH all_constants_shader ENTRY_POINT foo + COMPILE_OPTIONS all_constants_shader + -work-dim -global-offset + END + BIND BUFFER all_constants_buf AS storage DESCRIPTOR_SET 0 BINDING 0 +END + +RUN work_dim_pipeline 1 1 1 +EXPECT work_dim_buf IDX 0 EQ 3 + +RUN global_offset_pipeline 1 1 1 +EXPECT global_offset_buf IDX 0 EQ 0 +EXPECT global_offset_buf IDX 4 EQ 0 +EXPECT global_offset_buf IDX 8 EQ 0 + +RUN all_constants_pipeline 1 1 1 +EXPECT all_constants_buf IDX 0 EQ 0 +EXPECT all_constants_buf IDX 4 EQ 0 +EXPECT all_constants_buf IDX 8 EQ 0 +EXPECT all_constants_buf IDX 12 EQ 3 diff --git a/tests/run_tests.py b/tests/run_tests.py index 7d760509c..b3903cdd8 100755 --- a/tests/run_tests.py +++ b/tests/run_tests.py @@ -95,6 +95,7 @@ OPENCL_CASES = [ "opencl_bind_buffer.amber", "opencl_c_copy.amber", + "opencl_generated_push_constants.amber", "opencl_read_and_write_image3d_rgba32i.amber", "opencl_read_image.amber", "opencl_read_image_literal_sampler.amber", From 3d6de1c75987ffc3a149f0169f896cafbbc5f213 Mon Sep 17 00:00:00 2001 From: James Price Date: Thu, 12 Mar 2020 12:19:53 -0400 Subject: [PATCH 2/2] Address review comments and bot failures --- src/clspv_helper.cc | 6 +++--- src/pipeline.cc | 32 +++++++++++++++++--------------- src/pipeline.h | 9 +++++---- src/pipeline_test.cc | 7 ++++--- 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/clspv_helper.cc b/src/clspv_helper.cc index b429a7d1b..32edd0783 100644 --- a/src/clspv_helper.cc +++ b/src/clspv_helper.cc @@ -100,11 +100,11 @@ Result Compile(Pipeline::ShaderInfo* shader_info, switch (entry.push_constant_data.pc) { case clspv::PushConstant::Dimensions: push_constant.type = - Pipeline::ShaderInfo::PushConstant::kPushConstantDimensions; + Pipeline::ShaderInfo::PushConstant::PushConstantType::kDimensions; break; case clspv::PushConstant::GlobalOffset: - push_constant.type = - Pipeline::ShaderInfo::PushConstant::kPushConstantGlobalOffset; + push_constant.type = Pipeline::ShaderInfo::PushConstant:: + PushConstantType::kGlobalOffset; break; default: return Result("unhandled push constant generated by Clspv"); diff --git a/src/pipeline.cc b/src/pipeline.cc index 951246bd9..3830436a7 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -15,6 +15,7 @@ #include "src/pipeline.h" #include +#include #include #include @@ -837,27 +838,27 @@ Result Pipeline::GenerateOpenCLPushConstants() { return {}; // Determine size and contents of the push constant buffer. - std::vector bytes; + std::vector bytes; for (const auto& pc : shader_info.GetPushConstants()) { - if (pc.offset + pc.size > bytes.size()) { - bytes.resize(pc.offset + pc.size); + 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); switch (pc.type) { - case Pipeline::ShaderInfo::PushConstant::kPushConstantDimensions: + case Pipeline::ShaderInfo::PushConstant::PushConstantType::kDimensions: // All compute kernel launches are 3D. - assert(pc.size == 4); - *reinterpret_cast(bytes.data() + pc.offset) = 3; + bytes[base] = 3; break; - case Pipeline::ShaderInfo::PushConstant::kPushConstantGlobalOffset: { + case Pipeline::ShaderInfo::PushConstant::PushConstantType::kGlobalOffset: // Global offsets are not currently supported. - assert(pc.size == 12); - uint32_t* ptr = reinterpret_cast(bytes.data() + pc.offset); - ptr[0] = 0; - ptr[1] = 0; - ptr[2] = 0; + bytes[base] = 0; + bytes[base + 1] = 0; + bytes[base + 2] = 0; break; - } } } @@ -869,8 +870,9 @@ Result Pipeline::GenerateOpenCLPushConstants() { std::unique_ptr buf = MakeUnique(); buf->SetName(kGeneratedPushConstantBuffer); buf->SetFormat(fmt.get()); - buf->SetSizeInBytes(bytes.size()); - buf->ValuePtr()->assign(bytes.begin(), bytes.end()); + 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()) diff --git a/src/pipeline.h b/src/pipeline.h index 7cf595047..b565eca62 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -114,10 +114,11 @@ class Pipeline { /// Push constant information for an OpenCL-C shader. struct PushConstant { - enum { - kPushConstantDimensions, - kPushConstantGlobalOffset, - } type; + enum class PushConstantType { + kDimensions = 0, + kGlobalOffset, + }; + PushConstantType type; uint32_t offset = 0; uint32_t size = 0; }; diff --git a/src/pipeline_test.cc b/src/pipeline_test.cc index 513346c06..de7736996 100644 --- a/src/pipeline_test.cc +++ b/src/pipeline_test.cc @@ -831,13 +831,14 @@ TEST_F(PipelineTest, OpenCLGeneratePushConstants) { p.SetShaderEntryPoint(&cs, "my_main"); Pipeline::ShaderInfo::PushConstant pc1; - pc1.type = Pipeline::ShaderInfo::PushConstant::kPushConstantDimensions; + pc1.type = Pipeline::ShaderInfo::PushConstant::PushConstantType::kDimensions; pc1.offset = 0; pc1.size = 4; p.GetShaders()[0].AddPushConstant(std::move(pc1)); Pipeline::ShaderInfo::PushConstant pc2; - pc2.type = Pipeline::ShaderInfo::PushConstant::kPushConstantGlobalOffset; + pc2.type = + Pipeline::ShaderInfo::PushConstant::PushConstantType::kGlobalOffset; pc2.offset = 16; pc2.size = 12; p.GetShaders()[0].AddPushConstant(std::move(pc2)); @@ -848,7 +849,7 @@ TEST_F(PipelineTest, OpenCLGeneratePushConstants) { const auto& buf = p.GetPushConstantBuffer(); EXPECT_EQ(28U, buf.buffer->GetSizeInBytes()); - uint32_t* bytes = reinterpret_cast(buf.buffer->ValuePtr()->data()); + const uint32_t* bytes = buf.buffer->GetValues(); EXPECT_EQ(3U, bytes[0]); EXPECT_EQ(0U, bytes[4]); EXPECT_EQ(0U, bytes[5]);