From 91d21cf7a842b4fc2dcf9cc29188db5d2f267192 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Sat, 20 Jul 2019 11:02:31 -0400 Subject: [PATCH 01/11] Parse pipeline SET command for OpenCL POD args Fixes #428 * Adds parsing (and tests) for SET command in pipeline command * no actions yet SET KERNEL ARG_NAME _name_ AS {datum_type} _value_ SET KERNEL ARG_NUMBER _number_ AS {datum_type} _value_ --- src/CMakeLists.txt | 1 + src/amberscript/parser.cc | 55 +++++ src/amberscript/parser.h | 1 + src/amberscript/parser_pipeline_set_test.cc | 216 ++++++++++++++++++++ 4 files changed, 273 insertions(+) create mode 100644 src/amberscript/parser_pipeline_set_test.cc diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 03d183afa..cdf5c47b0 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -123,6 +123,7 @@ if (${AMBER_ENABLE_TESTS}) amberscript/parser_extension_test.cc amberscript/parser_framebuffer_test.cc amberscript/parser_pipeline_test.cc + amberscript/parser_pipeline_set_test.cc amberscript/parser_repeat_test.cc amberscript/parser_run_test.cc amberscript/parser_set_test.cc diff --git a/src/amberscript/parser.cc b/src/amberscript/parser.cc index 1abe3a4f1..81e2a1738 100644 --- a/src/amberscript/parser.cc +++ b/src/amberscript/parser.cc @@ -415,6 +415,8 @@ Result Parser::ParsePipelineBody(const std::string& cmd_name, r = ParsePipelineVertexData(pipeline.get()); } else if (tok == "INDEX_DATA") { r = ParsePipelineIndexData(pipeline.get()); + } else if (tok == "SET") { + r = ParsePipelineSet(pipeline.get()); } else { r = Result("unknown token in pipeline block: " + tok); } @@ -778,6 +780,59 @@ Result Parser::ParsePipelineIndexData(Pipeline* pipeline) { return ValidateEndOfStatement("INDEX_DATA command"); } +Result Parser::ParsePipelineSet(Pipeline* pipeline) { + auto token = tokenizer_->NextToken(); + if (!token->IsString() || token->AsString() != "KERNEL") + return Result("missing KERNEL in SET command"); + + token = tokenizer_->NextToken(); + if (!token->IsString()) + return Result("expected ARG_NAME or ARG_NUMBER"); + + std::string arg_name = ""; + uint32_t arg_no = std::numeric_limits::max(); + if (token->AsString() == "ARG_NAME") { + token = tokenizer_->NextToken(); + if (!token->IsString()) + return Result("expected argument identifier"); + arg_name = token->AsString(); + } else if (token->AsString() == "ARG_NUMBER") { + token = tokenizer_->NextToken(); + if (!token->IsInteger()) + return Result("expected argument number"); + arg_no = token->AsUint32(); + } else { + return Result("expected ARG_NAME or ARG_NUMBER"); + } + + token = tokenizer_->NextToken(); + if (!token->IsString() || token->AsString() != "AS") + return Result("missing AS in SET command"); + + token = tokenizer_->NextToken(); + if (!token->IsString()) + return Result("expected data type"); + + DatumType arg_type; + auto r = ToDatumType(token->AsString(), &arg_type); + if (!r.IsSuccess()) + return r; + + token = tokenizer_->NextToken(); + if (!token->IsInteger() && !token->IsDouble()) + return Result("expected data value"); + + Value value; + if (arg_type.IsFloat() || arg_type.IsDouble()) + value.SetDoubleValue(token->AsDouble()); + else + value.SetIntValue(token->AsUint64()); + + (void)pipeline; + //pipeline->SetPODValue(arg_name, arg_no, arg_type, value); + return ValidateEndOfStatement("SET command"); +} + Result Parser::ParseBuffer() { auto token = tokenizer_->NextToken(); if (!token->IsString()) diff --git a/src/amberscript/parser.h b/src/amberscript/parser.h index f5d7b46b7..028ae5cdc 100644 --- a/src/amberscript/parser.h +++ b/src/amberscript/parser.h @@ -62,6 +62,7 @@ class Parser : public amber::Parser { Result ParsePipelineBind(Pipeline*); Result ParsePipelineVertexData(Pipeline*); Result ParsePipelineIndexData(Pipeline*); + Result ParsePipelineSet(Pipeline*); Result ParseRun(); Result ParseClear(); Result ParseClearColor(); diff --git a/src/amberscript/parser_pipeline_set_test.cc b/src/amberscript/parser_pipeline_set_test.cc new file mode 100644 index 000000000..a236653a6 --- /dev/null +++ b/src/amberscript/parser_pipeline_set_test.cc @@ -0,0 +1,216 @@ +// Copyright 2019 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 +// +// http://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 parseried. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "gtest/gtest.h" +#include "src/amberscript/parser.h" + +namespace amber { +namespace amberscript { + +using AmberScriptParserTest = testing::Test; + +TEST_F(AmberScriptParserTest, OpenCLSetMissingKernel) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET ARG_NAME a AS uint32 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: missing KERNEL in SET command", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetMissingArgName) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL a AS uint32 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: expected ARG_NAME or ARG_NUMBER", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetMissingArgIdentifier) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NAME AS uint32 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: missing AS in SET command", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetMissingArgIdentifierNumber) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NUMBER AS uint32 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: expected argument number", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetMissingAs) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NAME a uint32 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: missing AS in SET command", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetMissingDataType) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NAME a AS 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: expected data type", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetMissingDataValue) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NAME a AS uint32 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("7: expected data value", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetExtraTokens) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NAME a AS uint32 0 BLAH +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: extra parameters after SET command", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetArgNameNotString) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NAME 0 AS uint32 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: expected argument identifier", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetArgNumberNotInteger) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NUMBER 1.0 AS uint32 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: expected argument number", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetDataTypeNotString) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NUMBER 0 AS 0 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: expected data type", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetDataValueString) { + std::string in = R"( +SHADER compute my_shader OPENCL-C +#shader +END +PIPELINE compute my_pipeline + SET KERNEL ARG_NUMBER 0 AS uint32 data +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("6: expected data value", r.Error()); +} + +} // namespace amberscript +} // namespace amber From e071c492c832cc24890978f22065642ada46160e Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 09:39:18 -0400 Subject: [PATCH 02/11] Simple struct to record set commands --- src/amberscript/parser.cc | 8 ++++++-- src/pipeline.h | 13 +++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/amberscript/parser.cc b/src/amberscript/parser.cc index 81e2a1738..8e3c455ef 100644 --- a/src/amberscript/parser.cc +++ b/src/amberscript/parser.cc @@ -828,8 +828,12 @@ Result Parser::ParsePipelineSet(Pipeline* pipeline) { else value.SetIntValue(token->AsUint64()); - (void)pipeline; - //pipeline->SetPODValue(arg_name, arg_no, arg_type, value); + Pipeline::ArgSetInfo info; + info.name = arg_name; + info.ordinal = arg_no; + info.type = arg_type; + info.value = value; + pipeline->SetArg(std::move(info)); return ValidateEndOfStatement("SET command"); } diff --git a/src/pipeline.h b/src/pipeline.h index 8859888c7..19c766ee0 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -226,6 +226,17 @@ class Pipeline { /// Generates a default depth attachment in D32_SFLOAT_S8_UINT format. std::unique_ptr GenerateDefaultDepthAttachmentBuffer() const; + /// Information on values set for OpenCL-C plain-old-data args. + struct ArgSetInfo { + std::string name = ""; + uint32_t ordinal = 0; + DatumType type; + Value value; + }; + + void SetArg(ArgSetInfo&& info) { set_arg_values_.push_back(std::move(info)); } + const std::vector& SetArgsValue() const { return set_arg_values_; } + private: void UpdateFramebufferSizes(); @@ -244,6 +255,8 @@ class Pipeline { uint32_t fb_width_ = 250; uint32_t fb_height_ = 250; + + std::vector set_arg_values_; }; } // namespace amber From f965a6792023fa1913d5707838680e6ad8e7ffa2 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 11:31:58 -0400 Subject: [PATCH 03/11] Generate the PoD argument buffers * Executor now invokes the pipeline to generate buffers for the PoD args populated from SET commands --- src/executor.cc | 4 +++ src/pipeline.cc | 90 +++++++++++++++++++++++++++++++++++++++++++++++++ src/pipeline.h | 9 ++++- 3 files changed, 102 insertions(+), 1 deletion(-) diff --git a/src/executor.cc b/src/executor.cc index 3bbec7a8d..308813471 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -59,8 +59,12 @@ Result Executor::Execute(Engine* engine, if (!r.IsSuccess()) return r; + for (auto& pipeline : script->GetPipelines()) { r = pipeline->UpdateOpenCLBufferBindings(); + if (!r.IsSuccess()) + return r; + r = pipeline->GenerateOpenCLPoDBuffers(); if (!r.IsSuccess()) return r; r = engine->CreatePipeline(pipeline.get()); diff --git a/src/pipeline.cc b/src/pipeline.cc index 8f214f0fa..00d2b5559 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -430,4 +430,94 @@ Result Pipeline::UpdateOpenCLBufferBindings() { return {}; } +Result Pipeline::GenerateOpenCLPoDBuffers() { + if (!IsCompute() || GetShaders().empty() || + GetShaders()[0].GetShader()->GetFormat() != kShaderFormatOpenCLC) + return {}; + + const auto& shader_info = GetShaders()[0]; + const auto& descriptor_map = shader_info.GetDescriptorMap(); + if (descriptor_map.empty()) + return {}; + + const auto iter = descriptor_map.find(shader_info.GetEntryPoint()); + if (iter == descriptor_map.end()) + return {}; + + for (const auto& arg_info : SetArgValues()) { + uint32_t descriptor_set = std::numeric_limits::max(); + uint32_t binding = std::numeric_limits::max(); + uint32_t offset = 0; + uint32_t arg_size = 0; + Pipeline::ShaderInfo::DescriptorMapEntry::Kind kind; + for (const auto& entry : iter->second) { + if (entry.kind != Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD && + entry.kind != Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_UBO) { + continue; + } + + // Found the right entry. + if (entry.arg_name == arg_info.name || entry.arg_ordinal == arg_info.ordinal) { + descriptor_set = entry.descriptor_set; + binding = entry.binding; + offset = entry.pod_offset; + arg_size = entry.pod_arg_size; + kind = entry.kind; + break; + } + } + + if (descriptor_set == std::numeric_limits::max() || + binding == std::numeric_limits::max()) { + return Result("could not find descriptor map entry for SET command"); + } + + 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"); + } + + // Add a new buffer for this descriptor set and binding. + opencl_pod_buffers_.push_back(MakeUnique()); + buffer = opencl_pod_buffers_.back().get(); + buffer->SetBufferType( + kind == Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD + ? BufferType::kStorage + : BufferType::kUniform); + DatumType char_type; + char_type.SetType(DataType::kUint8); + buffer->SetFormat(char_type.AsFormat()); + 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, descriptor_set, binding); + } else { + buffer = buf_iter->second; + } + + // Resize if necessary. + if (buffer->ValueCount() < offset + arg_size) { + buffer->ResizeTo(offset + arg_size); + } + // Check the data size. + if (arg_size != arg_info.type.SizeInBytes()) + return Result("SET command uses incorrect data size"); + buffer->SetDataWithOffset({arg_info.value}, offset); + } + + return {}; +} + } // namespace amber diff --git a/src/pipeline.h b/src/pipeline.h index 19c766ee0..3d93cfb50 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -234,8 +234,13 @@ class Pipeline { Value value; }; + /// Adds value from SET command. void SetArg(ArgSetInfo&& info) { set_arg_values_.push_back(std::move(info)); } - const std::vector& SetArgsValue() const { return set_arg_values_; } + const std::vector& SetArgValues() const { return set_arg_values_; } + + /// Generate the buffers necessary for OpenCL PoD arguments populated via SET + /// command. + Result GenerateOpenCLPoDBuffers(); private: void UpdateFramebufferSizes(); @@ -257,6 +262,8 @@ class Pipeline { uint32_t fb_height_ = 250; std::vector set_arg_values_; + std::vector> opencl_pod_buffers_; + std::map, Buffer*> opencl_pod_buffer_map_; }; } // namespace amber From 1ddba4528eefe3d7caebf9431ae544b65cea631d Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 13:55:35 -0400 Subject: [PATCH 04/11] Fixes and improvements * Allow clustered pod args * Add some helper comments and more useful errors * tests for buffer generation * refactor opencl specific code in executor to its own loop --- src/clspv_helper.cc | 2 - src/executor.cc | 6 +- src/pipeline.cc | 41 +++++++++-- src/pipeline.h | 6 +- src/pipeline_test.cc | 170 ++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 209 insertions(+), 16 deletions(-) diff --git a/src/clspv_helper.cc b/src/clspv_helper.cc index e0d853ea6..c4ac35ac8 100644 --- a/src/clspv_helper.cc +++ b/src/clspv_helper.cc @@ -68,8 +68,6 @@ 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) { - if (entry.kernel_arg_data.pod_offset != 0) - return Result("Clustered PoD arguments are not currently supported"); descriptor_entry.pod_offset = entry.kernel_arg_data.pod_offset; descriptor_entry.pod_arg_size = entry.kernel_arg_data.pod_arg_size; } diff --git a/src/executor.cc b/src/executor.cc index 308813471..dc6335055 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -60,13 +60,17 @@ Result Executor::Execute(Engine* engine, return r; - for (auto& pipeline : script->GetPipelines()) { + // OpenCL specific pipeline updates. + for (auto& pipeline: script->GetPipelines()) { r = pipeline->UpdateOpenCLBufferBindings(); if (!r.IsSuccess()) return r; r = pipeline->GenerateOpenCLPoDBuffers(); if (!r.IsSuccess()) return r; + } + + for (auto& pipeline : script->GetPipelines()) { r = engine->CreatePipeline(pipeline.get()); if (!r.IsSuccess()) return r; diff --git a/src/pipeline.cc b/src/pipeline.cc index 00d2b5559..e4d72a5d8 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -444,20 +444,27 @@ Result Pipeline::GenerateOpenCLPoDBuffers() { if (iter == descriptor_map.end()) return {}; + // For each SET command, do the following: + // 1. Find the descriptor map entry for that argument. + // 2. Find or create the buffer for the descriptor set and binding pair. + // 3. Write the data for the SET command at the right offset. for (const auto& arg_info : SetArgValues()) { uint32_t descriptor_set = std::numeric_limits::max(); uint32_t binding = std::numeric_limits::max(); uint32_t offset = 0; uint32_t arg_size = 0; + bool uses_name = !arg_info.name.empty(); Pipeline::ShaderInfo::DescriptorMapEntry::Kind kind; for (const auto& entry : iter->second) { if (entry.kind != Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD && - entry.kind != Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_UBO) { + entry.kind != + Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD_UBO) { continue; } // Found the right entry. - if (entry.arg_name == arg_info.name || entry.arg_ordinal == arg_info.ordinal) { + if (entry.arg_name == arg_info.name || + entry.arg_ordinal == arg_info.ordinal) { descriptor_set = entry.descriptor_set; binding = entry.binding; offset = entry.pod_offset; @@ -469,10 +476,19 @@ Result Pipeline::GenerateOpenCLPoDBuffers() { if (descriptor_set == std::numeric_limits::max() || binding == std::numeric_limits::max()) { - return Result("could not find descriptor map entry for SET command"); + 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)); + 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 || @@ -483,7 +499,10 @@ Result Pipeline::GenerateOpenCLPoDBuffers() { if (buf_info.descriptor_set == descriptor_set && buf_info.binding == binding) return Result("previously bound buffer " + - buf_info.buffer->GetName() + " to PoD args"); + buf_info.buffer->GetName() + + " to PoD args at descriptor set " + + std::to_string(descriptor_set) + " binding " + + std::to_string(binding)); } // Add a new buffer for this descriptor set and binding. @@ -512,8 +531,16 @@ Result Pipeline::GenerateOpenCLPoDBuffers() { buffer->ResizeTo(offset + arg_size); } // Check the data size. - if (arg_size != arg_info.type.SizeInBytes()) - return Result("SET command uses incorrect data size"); + if (arg_size != arg_info.type.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); + } buffer->SetDataWithOffset({arg_info.value}, offset); } diff --git a/src/pipeline.h b/src/pipeline.h index 3d93cfb50..840d2b227 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -236,10 +236,12 @@ class Pipeline { /// Adds value from SET command. void SetArg(ArgSetInfo&& info) { set_arg_values_.push_back(std::move(info)); } - const std::vector& SetArgValues() const { return set_arg_values_; } + const std::vector& SetArgValues() const { + return set_arg_values_; + } /// Generate the buffers necessary for OpenCL PoD arguments populated via SET - /// command. + /// command. This should be called after all other buffers are bound. Result GenerateOpenCLPoDBuffers(); private: diff --git a/src/pipeline_test.cc b/src/pipeline_test.cc index 8e0ddccc2..272e22559 100644 --- a/src/pipeline_test.cc +++ b/src/pipeline_test.cc @@ -396,8 +396,7 @@ TEST_F(PipelineTest, Clone) { EXPECT_EQ(2U, bufs[1].binding); } -#if AMBER_ENABLE_CLSPV -TEST_F(PipelineTest, ClspvUpdateBindings) { +TEST_F(PipelineTest, OpenCLUpdateBindings) { Pipeline p(PipelineType::kCompute); p.SetName("my_pipeline"); @@ -442,7 +441,7 @@ TEST_F(PipelineTest, ClspvUpdateBindings) { EXPECT_EQ(1U, bufs[1].binding); } -TEST_F(PipelineTest, ClspvUpdateBindingTypeMismatch) { +TEST_F(PipelineTest, OpenCLUpdateBindingTypeMismatch) { Pipeline p(PipelineType::kCompute); p.SetName("my_pipeline"); @@ -480,6 +479,169 @@ TEST_F(PipelineTest, ClspvUpdateBindingTypeMismatch) { ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("Buffer buf2 must be an uniform binding", r.Error()); } -#endif // AMBER_ENABLE_CLSPV + +TEST_F(PipelineTest, OpenCLGeneratePoDBuffers) { + 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; + entry1.descriptor_set = 4; + entry1.binding = 5; + 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; + entry2.descriptor_set = 4; + entry2.binding = 5; + entry2.arg_name = "arg_b"; + entry2.arg_ordinal = 0; + 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; + entry3.descriptor_set = 4; + entry3.binding = 4; + entry3.arg_name = "arg_c"; + entry3.arg_ordinal = 0; + entry3.pod_offset = 0; + entry3.pod_arg_size = 4; + p.GetShaders()[0].AddDescriptorEntry("my_main", std::move(entry3)); + + // Set commands. + Value int_value; + int_value.SetIntValue(1); + DatumType int_type; + int_type.SetType(DataType::kInt32); + DatumType char_type; + char_type.SetType(DataType::kInt8); + + Pipeline::ArgSetInfo arg_info1; + arg_info1.name = "arg_a"; + arg_info1.ordinal = 99; + arg_info1.type = int_type; + 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.type = char_type; + 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.type = int_type; + arg_info3.value = int_value; + p.SetArg(std::move(arg_info3)); + + auto r = p.GenerateOpenCLPoDBuffers(); + ASSERT_TRUE(r.IsSuccess()); + EXPECT_EQ(2U, p.GetBuffers().size()); + + const auto& b1 = p.GetBuffers()[0]; + EXPECT_EQ(4U, b1.descriptor_set); + EXPECT_EQ(5U, b1.binding); + EXPECT_EQ(5U, b1.buffer->ValueCount()); + + const auto& b2 = p.GetBuffers()[1]; + EXPECT_EQ(4U, b2.descriptor_set); + EXPECT_EQ(4U, b2.binding); + EXPECT_EQ(4U, b2.buffer->ValueCount()); +} + +TEST_F(PipelineTest, OpenCLGeneratePoDBuffersBadName) { + 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; + entry1.descriptor_set = 4; + entry1.binding = 5; + 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)); + + // Set commands. + Value int_value; + int_value.SetIntValue(1); + DatumType int_type; + int_type.SetType(DataType::kInt32); + + Pipeline::ArgSetInfo arg_info1; + arg_info1.name = "arg_z"; + arg_info1.ordinal = 99; + arg_info1.type = int_type; + arg_info1.value = int_value; + p.SetArg(std::move(arg_info1)); + + auto r = p.GenerateOpenCLPoDBuffers(); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ( + "could not find descriptor map entry for SET command: kernel my_main, " + "name arg_z", + r.Error()); +} + +TEST_F(PipelineTest, OpenCLGeneratePoDBuffersBadSize) { + 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; + entry1.descriptor_set = 4; + entry1.binding = 5; + 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)); + + // Set commands. + Value int_value; + int_value.SetIntValue(1); + DatumType short_type; + short_type.SetType(DataType::kInt16); + + Pipeline::ArgSetInfo arg_info1; + arg_info1.name = ""; + arg_info1.ordinal = 0; + arg_info1.type = short_type; + arg_info1.value = int_value; + p.SetArg(std::move(arg_info1)); + + auto r = p.GenerateOpenCLPoDBuffers(); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("SET command uses incorrect data size: kernel my_main, number 0", + r.Error()); +} } // namespace amber From 973fecb38c604ba509299902cd322a88fe080c73 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 13:57:55 -0400 Subject: [PATCH 05/11] Add end-to-end test --- tests/cases/opencl_set_arg.amber | 35 ++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 tests/cases/opencl_set_arg.amber diff --git a/tests/cases/opencl_set_arg.amber b/tests/cases/opencl_set_arg.amber new file mode 100644 index 000000000..75fb9952a --- /dev/null +++ b/tests/cases/opencl_set_arg.amber @@ -0,0 +1,35 @@ +#!amber +# Copyright 2019 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 my_shader OPENCL-C +kernel void line(global int* in, global int* out, int slope, int offset) { + *out = *in * slope + offset; +} +END + +BUFFER in_buf DATA_TYPE uint32 DATA 3 END +BUFFER out_buf DATA_TYPE uint32 DATA 0 END + +PIPELINE compute my_pipeline + ATTACH my_shader ENTRY_POINT line + BIND BUFFER in_buf KERNEL ARG_NAME in + BIND BUFFER out_buf KERNEL ARG_NAME out + SET KERNEL ARG_NAME offset AS uint32 1 + SET KERNEL ARG_NAME slope AS int32 2 +END + +RUN my_pipeline 1 1 1 + +EXPECT out_buf IDX 0 EQ 7 From 18ffd5f9ce9a41f8980ab7ca5d898b0f8b966b21 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 14:27:14 -0400 Subject: [PATCH 06/11] Document SET command --- docs/amber_script.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/amber_script.md b/docs/amber_script.md index a7a552c0b..aeace0c48 100644 --- a/docs/amber_script.md +++ b/docs/amber_script.md @@ -275,6 +275,20 @@ attachment content, depth/stencil content, uniform buffers, etc. INDEX_DATA {buffer_name} ``` +##### OpenCL Plain-Old-Data Arguments +OpenCL kernels can have plain-old-data (pod or pod_ubo in the desriptor map) +arguments set their data via this command. Amber will generate the appropriate +buffers for the pipeline populated with the specified data. + +```groovy + # Set argument |name| to |data_type| with value |val|. + SET KERNEL ARG_NAME _name_ AS {data_type} _val_ + + # Set argument number |number| to |data_type| with value |val|. + # Arguments use 0-based numbering. + SET KERNEL ARG_NUMBER _number_ AS {data_type} _val_ +``` + ##### Topologies * `point_list` * `line_list` From cafb9fe3c79629b6757b5bfd2aeb730a4631277a Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 14:44:56 -0400 Subject: [PATCH 07/11] formatting and build fix --- src/executor.cc | 2 +- src/pipeline.cc | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/executor.cc b/src/executor.cc index dc6335055..de3e7b53d 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -61,7 +61,7 @@ Result Executor::Execute(Engine* engine, // OpenCL specific pipeline updates. - for (auto& pipeline: script->GetPipelines()) { + for (auto& pipeline : script->GetPipelines()) { r = pipeline->UpdateOpenCLBufferBindings(); if (!r.IsSuccess()) return r; diff --git a/src/pipeline.cc b/src/pipeline.cc index e4d72a5d8..c7b1fe029 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -454,7 +454,8 @@ Result Pipeline::GenerateOpenCLPoDBuffers() { uint32_t offset = 0; uint32_t arg_size = 0; bool uses_name = !arg_info.name.empty(); - Pipeline::ShaderInfo::DescriptorMapEntry::Kind kind; + Pipeline::ShaderInfo::DescriptorMapEntry::Kind kind = + Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD; for (const auto& entry : iter->second) { if (entry.kind != Pipeline::ShaderInfo::DescriptorMapEntry::Kind::POD && entry.kind != From 9d18cdb1fe1865bcfc4b5db9e61bf29abfd1b72f Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 14:51:42 -0400 Subject: [PATCH 08/11] formatting --- src/executor.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/executor.cc b/src/executor.cc index de3e7b53d..918acc063 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -59,7 +59,6 @@ Result Executor::Execute(Engine* engine, if (!r.IsSuccess()) return r; - // OpenCL specific pipeline updates. for (auto& pipeline : script->GetPipelines()) { r = pipeline->UpdateOpenCLBufferBindings(); From 6a9e0c0a54d9cd78433678683685faa6d73ad67d Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 15:29:37 -0400 Subject: [PATCH 09/11] changes for review --- docs/amber_script.md | 2 +- src/executor.cc | 2 +- src/pipeline.cc | 10 ++++++---- src/pipeline.h | 3 ++- src/pipeline_test.cc | 12 ++++++------ 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/docs/amber_script.md b/docs/amber_script.md index aeace0c48..93e97bab2 100644 --- a/docs/amber_script.md +++ b/docs/amber_script.md @@ -284,7 +284,7 @@ buffers for the pipeline populated with the specified data. # Set argument |name| to |data_type| with value |val|. SET KERNEL ARG_NAME _name_ AS {data_type} _val_ - # Set argument number |number| to |data_type| with value |val|. + # Set argument |number| to |data_type| with value |val|. # Arguments use 0-based numbering. SET KERNEL ARG_NUMBER _number_ AS {data_type} _val_ ``` diff --git a/src/executor.cc b/src/executor.cc index 918acc063..1c3dd89ab 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -64,7 +64,7 @@ Result Executor::Execute(Engine* engine, r = pipeline->UpdateOpenCLBufferBindings(); if (!r.IsSuccess()) return r; - r = pipeline->GenerateOpenCLPoDBuffers(); + r = pipeline->GenerateOpenCLPodBuffers(); if (!r.IsSuccess()) return r; } diff --git a/src/pipeline.cc b/src/pipeline.cc index c7b1fe029..650e2534e 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -430,10 +430,11 @@ Result Pipeline::UpdateOpenCLBufferBindings() { return {}; } -Result Pipeline::GenerateOpenCLPoDBuffers() { +Result Pipeline::GenerateOpenCLPodBuffers() { if (!IsCompute() || GetShaders().empty() || - GetShaders()[0].GetShader()->GetFormat() != kShaderFormatOpenCLC) + GetShaders()[0].GetShader()->GetFormat() != kShaderFormatOpenCLC) { return {}; + } const auto& shader_info = GetShaders()[0]; const auto& descriptor_map = shader_info.GetDescriptorMap(); @@ -464,7 +465,7 @@ Result Pipeline::GenerateOpenCLPoDBuffers() { } // Found the right entry. - if (entry.arg_name == arg_info.name || + if ((uses_name && entry.arg_name == arg_info.name) || entry.arg_ordinal == arg_info.ordinal) { descriptor_set = entry.descriptor_set; binding = entry.binding; @@ -498,12 +499,13 @@ Result Pipeline::GenerateOpenCLPoDBuffers() { // binding pair. for (const auto& buf_info : GetBuffers()) { if (buf_info.descriptor_set == descriptor_set && - buf_info.binding == binding) + 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)); + } } // Add a new buffer for this descriptor set and binding. diff --git a/src/pipeline.h b/src/pipeline.h index 840d2b227..52ce62017 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -242,7 +242,7 @@ class Pipeline { /// Generate the buffers necessary for OpenCL PoD arguments populated via SET /// command. This should be called after all other buffers are bound. - Result GenerateOpenCLPoDBuffers(); + Result GenerateOpenCLPodBuffers(); private: void UpdateFramebufferSizes(); @@ -265,6 +265,7 @@ class Pipeline { std::vector set_arg_values_; std::vector> opencl_pod_buffers_; + /// Maps (descriptor set, binding) to the buffer for that binding pair. std::map, Buffer*> opencl_pod_buffer_map_; }; diff --git a/src/pipeline_test.cc b/src/pipeline_test.cc index 272e22559..8a0d2a53d 100644 --- a/src/pipeline_test.cc +++ b/src/pipeline_test.cc @@ -480,7 +480,7 @@ TEST_F(PipelineTest, OpenCLUpdateBindingTypeMismatch) { EXPECT_EQ("Buffer buf2 must be an uniform binding", r.Error()); } -TEST_F(PipelineTest, OpenCLGeneratePoDBuffers) { +TEST_F(PipelineTest, OpenCLGeneratePodBuffers) { Pipeline p(PipelineType::kCompute); p.SetName("my_pipeline"); @@ -549,7 +549,7 @@ TEST_F(PipelineTest, OpenCLGeneratePoDBuffers) { arg_info3.value = int_value; p.SetArg(std::move(arg_info3)); - auto r = p.GenerateOpenCLPoDBuffers(); + auto r = p.GenerateOpenCLPodBuffers(); ASSERT_TRUE(r.IsSuccess()); EXPECT_EQ(2U, p.GetBuffers().size()); @@ -564,7 +564,7 @@ TEST_F(PipelineTest, OpenCLGeneratePoDBuffers) { EXPECT_EQ(4U, b2.buffer->ValueCount()); } -TEST_F(PipelineTest, OpenCLGeneratePoDBuffersBadName) { +TEST_F(PipelineTest, OpenCLGeneratePodBuffersBadName) { Pipeline p(PipelineType::kCompute); p.SetName("my_pipeline"); @@ -597,7 +597,7 @@ TEST_F(PipelineTest, OpenCLGeneratePoDBuffersBadName) { arg_info1.value = int_value; p.SetArg(std::move(arg_info1)); - auto r = p.GenerateOpenCLPoDBuffers(); + auto r = p.GenerateOpenCLPodBuffers(); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ( "could not find descriptor map entry for SET command: kernel my_main, " @@ -605,7 +605,7 @@ TEST_F(PipelineTest, OpenCLGeneratePoDBuffersBadName) { r.Error()); } -TEST_F(PipelineTest, OpenCLGeneratePoDBuffersBadSize) { +TEST_F(PipelineTest, OpenCLGeneratePodBuffersBadSize) { Pipeline p(PipelineType::kCompute); p.SetName("my_pipeline"); @@ -638,7 +638,7 @@ TEST_F(PipelineTest, OpenCLGeneratePoDBuffersBadSize) { arg_info1.value = int_value; p.SetArg(std::move(arg_info1)); - auto r = p.GenerateOpenCLPoDBuffers(); + auto r = p.GenerateOpenCLPodBuffers(); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("SET command uses incorrect data size: kernel my_main, number 0", r.Error()); From 71780311962bdbfc71e7db90364f9c2925442320 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 15:42:44 -0400 Subject: [PATCH 10/11] Changes for review * Remove empty string default * Add parser error if SET is used with non-OPENCL-C shaders * update tests --- src/amberscript/parser.cc | 6 +++ src/amberscript/parser_pipeline_set_test.cc | 53 ++++++++++++++++----- src/pipeline.h | 2 +- 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/amberscript/parser.cc b/src/amberscript/parser.cc index 8e3c455ef..82b898743 100644 --- a/src/amberscript/parser.cc +++ b/src/amberscript/parser.cc @@ -781,6 +781,12 @@ Result Parser::ParsePipelineIndexData(Pipeline* pipeline) { } Result Parser::ParsePipelineSet(Pipeline* pipeline) { + if (pipeline->GetShaders().empty() || + pipeline->GetShaders()[0].GetShader()->GetFormat() != + kShaderFormatOpenCLC) { + return Result("SET can only be used with OPENCL-C shaders"); + } + auto token = tokenizer_->NextToken(); if (!token->IsString() || token->AsString() != "KERNEL") return Result("missing KERNEL in SET command"); diff --git a/src/amberscript/parser_pipeline_set_test.cc b/src/amberscript/parser_pipeline_set_test.cc index a236653a6..b35b35e3a 100644 --- a/src/amberscript/parser_pipeline_set_test.cc +++ b/src/amberscript/parser_pipeline_set_test.cc @@ -26,6 +26,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET ARG_NAME a AS uint32 0 END )"; @@ -33,7 +34,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: missing KERNEL in SET command", r.Error()); + EXPECT_EQ("7: missing KERNEL in SET command", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetMissingArgName) { @@ -42,6 +43,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL a AS uint32 0 END )"; @@ -49,7 +51,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: expected ARG_NAME or ARG_NUMBER", r.Error()); + EXPECT_EQ("7: expected ARG_NAME or ARG_NUMBER", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetMissingArgIdentifier) { @@ -58,6 +60,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NAME AS uint32 0 END )"; @@ -65,7 +68,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: missing AS in SET command", r.Error()); + EXPECT_EQ("7: missing AS in SET command", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetMissingArgIdentifierNumber) { @@ -74,6 +77,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NUMBER AS uint32 0 END )"; @@ -81,7 +85,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: expected argument number", r.Error()); + EXPECT_EQ("7: expected argument number", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetMissingAs) { @@ -90,6 +94,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NAME a uint32 0 END )"; @@ -97,7 +102,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: missing AS in SET command", r.Error()); + EXPECT_EQ("7: missing AS in SET command", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetMissingDataType) { @@ -106,6 +111,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NAME a AS 0 END )"; @@ -113,7 +119,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: expected data type", r.Error()); + EXPECT_EQ("7: expected data type", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetMissingDataValue) { @@ -122,6 +128,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NAME a AS uint32 END )"; @@ -129,7 +136,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("7: expected data value", r.Error()); + EXPECT_EQ("8: expected data value", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetExtraTokens) { @@ -138,6 +145,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NAME a AS uint32 0 BLAH END )"; @@ -145,7 +153,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: extra parameters after SET command", r.Error()); + EXPECT_EQ("7: extra parameters after SET command", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetArgNameNotString) { @@ -154,6 +162,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NAME 0 AS uint32 0 END )"; @@ -161,7 +170,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: expected argument identifier", r.Error()); + EXPECT_EQ("7: expected argument identifier", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetArgNumberNotInteger) { @@ -170,6 +179,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NUMBER 1.0 AS uint32 0 END )"; @@ -177,7 +187,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: expected argument number", r.Error()); + EXPECT_EQ("7: expected argument number", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetDataTypeNotString) { @@ -186,6 +196,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NUMBER 0 AS 0 0 END )"; @@ -193,7 +204,7 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: expected data type", r.Error()); + EXPECT_EQ("7: expected data type", r.Error()); } TEST_F(AmberScriptParserTest, OpenCLSetDataValueString) { @@ -202,6 +213,7 @@ SHADER compute my_shader OPENCL-C #shader END PIPELINE compute my_pipeline + ATTACH my_shader SET KERNEL ARG_NUMBER 0 AS uint32 data END )"; @@ -209,7 +221,24 @@ END Parser parser; auto r = parser.Parse(in); ASSERT_FALSE(r.IsSuccess()); - EXPECT_EQ("6: expected data value", r.Error()); + EXPECT_EQ("7: expected data value", r.Error()); +} + +TEST_F(AmberScriptParserTest, OpenCLSetWrongShaderFormat) { + std::string in = R"( +SHADER compute my_shader SPIRV-ASM +#shader +END +PIPELINE compute my_pipeline + ATTACH my_shader + SET KERNEL ARG_NAME arg_a AS uint32 0 +END +)"; + + Parser parser; + auto r = parser.Parse(in); + ASSERT_FALSE(r.IsSuccess()); + EXPECT_EQ("7: SET can only be used with OPENCL-C shaders", r.Error()); } } // namespace amberscript diff --git a/src/pipeline.h b/src/pipeline.h index 52ce62017..7ea8f9a8b 100644 --- a/src/pipeline.h +++ b/src/pipeline.h @@ -228,7 +228,7 @@ class Pipeline { /// Information on values set for OpenCL-C plain-old-data args. struct ArgSetInfo { - std::string name = ""; + std::string name; uint32_t ordinal = 0; DatumType type; Value value; From 2878fab6f65eb95b557e57685e300be5bba34ea6 Mon Sep 17 00:00:00 2001 From: Alan Baker Date: Mon, 22 Jul 2019 15:49:39 -0400 Subject: [PATCH 11/11] explanatory comment --- src/pipeline.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pipeline.cc b/src/pipeline.cc index 650e2534e..08a846973 100644 --- a/src/pipeline.cc +++ b/src/pipeline.cc @@ -515,6 +515,8 @@ Result Pipeline::GenerateOpenCLPodBuffers() { 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. DatumType char_type; char_type.SetType(DataType::kUint8); buffer->SetFormat(char_type.AsFormat());