From 076c750b3f4531da6c29fc4a944c81e4134d37a1 Mon Sep 17 00:00:00 2001 From: Paul Thomson Date: Fri, 2 Aug 2019 14:18:19 +0100 Subject: [PATCH 1/4] Add option to disable SPIR-V validation --- include/amber/amber.h | 3 +++ samples/amber.cc | 5 +++++ src/amber.cc | 6 ++++-- src/executor.cc | 11 +++++++---- src/executor.h | 7 +++++-- src/shader_compiler.cc | 12 ++++++++---- src/shader_compiler.h | 3 ++- 7 files changed, 34 insertions(+), 13 deletions(-) diff --git a/include/amber/amber.h b/include/amber/amber.h index 6c8835a87..06cfaf348 100644 --- a/include/amber/amber.h +++ b/include/amber/amber.h @@ -105,6 +105,9 @@ struct Options { bool pipeline_create_only; /// Delegate implementation Delegate* delegate; + /// If true, disables SPIR-V validation. If false, SPIR-V shaders will be + /// validated using the Validator component (spirv-val) from SPIRV-Tools. + bool disable_spirv_validation; }; /// Main interface to the Amber environment. diff --git a/samples/amber.cc b/samples/amber.cc index 8c945b10d..a395f959f 100644 --- a/samples/amber.cc +++ b/samples/amber.cc @@ -56,6 +56,7 @@ struct Options { bool log_graphics_calls = false; bool log_graphics_calls_time = false; bool log_execute_calls = false; + bool disable_spirv_validation = false; amber::EngineType engine = amber::kEngineTypeVulkan; std::string spv_env; }; @@ -86,6 +87,7 @@ const char kUsage[] = R"(Usage: amber [options] SCRIPT [SCRIPTS...] --log-graphics-calls -- Log graphics API calls (only for Vulkan so far). --log-graphics-calls-time -- Log timing of graphics API calls timing (Vulkan only). --log-execute-calls -- Log each execute call before run. + --disable-spirv-val -- Disable SPIR-V validation. -h -- This help text. )"; @@ -193,6 +195,8 @@ bool ParseArgs(const std::vector& args, Options* opts) { opts->log_graphics_calls_time = true; } else if (arg == "--log-execute-calls") { opts->log_execute_calls = true; + } else if (arg == "--disable-spirv-val") { + opts->disable_spirv_validation = true; } else if (arg.size() > 0 && arg[0] == '-') { std::cerr << "Unrecognized option " << arg << std::endl; return false; @@ -354,6 +358,7 @@ int main(int argc, const char** argv) { amber_options.spv_env = options.spv_env; amber_options.pipeline_create_only = options.pipeline_create_only; amber_options.delegate = &delegate; + amber_options.disable_spirv_validation = options.disable_spirv_validation; std::set required_features; std::set required_device_extensions; diff --git a/src/amber.cc b/src/amber.cc index 183ebacfb..634fe4cef 100644 --- a/src/amber.cc +++ b/src/amber.cc @@ -68,7 +68,8 @@ Options::Options() : engine(amber::EngineType::kEngineTypeVulkan), config(nullptr), pipeline_create_only(false), - delegate(nullptr) {} + delegate(nullptr), + disable_spirv_validation(false) {} Options::~Options() = default; @@ -171,7 +172,8 @@ amber::Result Amber::ExecuteWithShaderData(const amber::Recipe* recipe, Result executor_result = executor.Execute( engine.get(), script, opts->delegate, shader_data, opts->pipeline_create_only ? ExecutionType::kPipelineCreateOnly - : ExecutionType::kExecute); + : ExecutionType::kExecute, + opts->disable_spirv_validation); // Hold the executor result until the extractions are complete. This will let // us dump any buffers requested even on failure. diff --git a/src/executor.cc b/src/executor.cc index 1c3dd89ab..3755b5e31 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -30,14 +30,16 @@ Executor::Executor() = default; Executor::~Executor() = default; Result Executor::CompileShaders(const amber::Script* script, - const ShaderMap& shader_map) { + const ShaderMap& shader_map, + bool disable_spirv_validation) { for (auto& pipeline : script->GetPipelines()) { for (auto& shader_info : pipeline->GetShaders()) { ShaderCompiler sc(script->GetSpvTargetEnv()); Result r; std::vector data; - std::tie(r, data) = sc.Compile(&shader_info, shader_map); + std::tie(r, data) = + sc.Compile(&shader_info, shader_map, disable_spirv_validation); if (!r.IsSuccess()) return r; @@ -51,11 +53,12 @@ Result Executor::Execute(Engine* engine, const amber::Script* script, Delegate* delegate, const ShaderMap& shader_map, - ExecutionType executionType) { + ExecutionType executionType, + bool disable_spirv_validation) { engine->SetEngineData(script->GetEngineData()); if (!script->GetPipelines().empty()) { - Result r = CompileShaders(script, shader_map); + Result r = CompileShaders(script, shader_map, disable_spirv_validation); if (!r.IsSuccess()) return r; diff --git a/src/executor.h b/src/executor.h index bdea471ff..89daf16e8 100644 --- a/src/executor.h +++ b/src/executor.h @@ -39,10 +39,13 @@ class Executor { const Script* script, Delegate* delegate, const ShaderMap& map, - ExecutionType executionType); + ExecutionType executionType, + bool disable_spirv_validation = false); private: - Result CompileShaders(const Script* script, const ShaderMap& shader_map); + Result CompileShaders(const Script* script, + const ShaderMap& shader_map, + bool disable_spirv_validation); Result ExecuteCommand(Engine* engine, Command* cmd); Verifier verifier_; diff --git a/src/shader_compiler.cc b/src/shader_compiler.cc index 10b7b86b9..9aca515f6 100644 --- a/src/shader_compiler.cc +++ b/src/shader_compiler.cc @@ -53,7 +53,8 @@ ShaderCompiler::~ShaderCompiler() = default; std::pair> ShaderCompiler::Compile( Pipeline::ShaderInfo* shader_info, - const ShaderMap& shader_map) const { + const ShaderMap& shader_map, + bool disable_spirv_validation) const { const auto shader = shader_info->GetShader(); auto it = shader_map.find(shader->GetName()); if (it != shader_map.end()) { @@ -142,10 +143,13 @@ std::pair> ShaderCompiler::Compile( return {Result("Invalid shader format"), results}; } + (void)disable_spirv_validation; #if AMBER_ENABLE_SPIRV_TOOLS - spvtools::ValidatorOptions options; - if (!tools.Validate(results.data(), results.size(), options)) - return {Result("Invalid shader: " + spv_errors), {}}; + if (!disable_spirv_validation) { + spvtools::ValidatorOptions options; + if (!tools.Validate(results.data(), results.size(), options)) + return {Result("Invalid shader: " + spv_errors), {}}; + } // Optimize the shader if any optimizations were specified. if (!shader_info->GetShaderOptimizations().empty()) { diff --git a/src/shader_compiler.h b/src/shader_compiler.h index f9566e529..b093383be 100644 --- a/src/shader_compiler.h +++ b/src/shader_compiler.h @@ -43,7 +43,8 @@ class ShaderCompiler { /// be invoked to produce the shader binary. std::pair> Compile( Pipeline::ShaderInfo* shader_info, - const ShaderMap& shader_map) const; + const ShaderMap& shader_map, + bool disable_spirv_validation = false) const; private: Result ParseHex(const std::string& data, std::vector* result) const; From f3305f1ec3276d6ca217f2b9aa9a66cb1a507961 Mon Sep 17 00:00:00 2001 From: Paul Thomson Date: Fri, 2 Aug 2019 15:57:19 +0100 Subject: [PATCH 2/4] Address comments. Pass down Options. Use member variable. --- include/amber/amber.h | 16 ++++-- samples/amber.cc | 4 +- src/amber.cc | 13 ++--- src/executor.cc | 21 ++++---- src/executor.h | 8 +-- src/executor_test.cc | 116 ++++++++++++++++++++--------------------- src/shader_compiler.cc | 10 ++-- src/shader_compiler.h | 7 +-- 8 files changed, 99 insertions(+), 96 deletions(-) diff --git a/include/amber/amber.h b/include/amber/amber.h index 06cfaf348..1fffd384d 100644 --- a/include/amber/amber.h +++ b/include/amber/amber.h @@ -38,6 +38,13 @@ enum EngineType { kEngineTypeDawn, }; +enum class ExecutionType { + /// Execute as normal. + kExecute = 0, + /// Only create the pipelines and then exit. + kPipelineCreateOnly +}; + /// Override point of engines to add their own configuration. struct EngineConfig { virtual ~EngineConfig(); @@ -101,13 +108,14 @@ struct Options { std::string spv_env; /// Lists the buffers to extract at the end of the execution std::vector extractions; - /// Terminate after creating the pipelines. - bool pipeline_create_only; - /// Delegate implementation - Delegate* delegate; + /// The type of execution. For example, execute as normal or just create the + /// piplines and exit. + ExecutionType execution_type; /// If true, disables SPIR-V validation. If false, SPIR-V shaders will be /// validated using the Validator component (spirv-val) from SPIRV-Tools. bool disable_spirv_validation; + /// Delegate implementation + Delegate* delegate; }; /// Main interface to the Amber environment. diff --git a/samples/amber.cc b/samples/amber.cc index a395f959f..b327bab91 100644 --- a/samples/amber.cc +++ b/samples/amber.cc @@ -356,7 +356,9 @@ int main(int argc, const char** argv) { amber::Options amber_options; amber_options.engine = options.engine; amber_options.spv_env = options.spv_env; - amber_options.pipeline_create_only = options.pipeline_create_only; + amber_options.execution_type = options.pipeline_create_only + ? amber::ExecutionType::kPipelineCreateOnly + : amber::ExecutionType::kExecute; amber_options.delegate = &delegate; amber_options.disable_spirv_validation = options.disable_spirv_validation; diff --git a/src/amber.cc b/src/amber.cc index 634fe4cef..65bcfc5bc 100644 --- a/src/amber.cc +++ b/src/amber.cc @@ -67,9 +67,9 @@ EngineConfig::~EngineConfig() = default; Options::Options() : engine(amber::EngineType::kEngineTypeVulkan), config(nullptr), - pipeline_create_only(false), - delegate(nullptr), - disable_spirv_validation(false) {} + execution_type(ExecutionType::kExecute), + disable_spirv_validation(false), + delegate(nullptr) {} Options::~Options() = default; @@ -169,11 +169,8 @@ amber::Result Amber::ExecuteWithShaderData(const amber::Recipe* recipe, script->SetSpvTargetEnv(opts->spv_env); Executor executor; - Result executor_result = executor.Execute( - engine.get(), script, opts->delegate, shader_data, - opts->pipeline_create_only ? ExecutionType::kPipelineCreateOnly - : ExecutionType::kExecute, - opts->disable_spirv_validation); + Result executor_result = + executor.Execute(engine.get(), script, shader_data, opts); // Hold the executor result until the extractions are complete. This will let // us dump any buffers requested even on failure. diff --git a/src/executor.cc b/src/executor.cc index 3755b5e31..2bd25f733 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -31,15 +31,15 @@ Executor::~Executor() = default; Result Executor::CompileShaders(const amber::Script* script, const ShaderMap& shader_map, - bool disable_spirv_validation) { + Options* options) { for (auto& pipeline : script->GetPipelines()) { for (auto& shader_info : pipeline->GetShaders()) { - ShaderCompiler sc(script->GetSpvTargetEnv()); + ShaderCompiler sc(script->GetSpvTargetEnv(), + options->disable_spirv_validation); Result r; std::vector data; - std::tie(r, data) = - sc.Compile(&shader_info, shader_map, disable_spirv_validation); + std::tie(r, data) = sc.Compile(&shader_info, shader_map); if (!r.IsSuccess()) return r; @@ -51,14 +51,12 @@ Result Executor::CompileShaders(const amber::Script* script, Result Executor::Execute(Engine* engine, const amber::Script* script, - Delegate* delegate, const ShaderMap& shader_map, - ExecutionType executionType, - bool disable_spirv_validation) { + Options* options) { engine->SetEngineData(script->GetEngineData()); if (!script->GetPipelines().empty()) { - Result r = CompileShaders(script, shader_map, disable_spirv_validation); + Result r = CompileShaders(script, shader_map, options); if (!r.IsSuccess()) return r; @@ -79,13 +77,14 @@ Result Executor::Execute(Engine* engine, } } - if (executionType == ExecutionType::kPipelineCreateOnly) + if (options->execution_type == ExecutionType::kPipelineCreateOnly) return {}; // Process Commands for (const auto& cmd : script->GetCommands()) { - if (delegate && delegate->LogExecuteCalls()) - delegate->Log(std::to_string(cmd->GetLine()) + ": " + cmd->ToString()); + if (options->delegate && options->delegate->LogExecuteCalls()) + options->delegate->Log(std::to_string(cmd->GetLine()) + ": " + + cmd->ToString()); Result r = ExecuteCommand(engine, cmd.get()); if (!r.IsSuccess()) diff --git a/src/executor.h b/src/executor.h index 89daf16e8..de5c794b5 100644 --- a/src/executor.h +++ b/src/executor.h @@ -23,8 +23,6 @@ namespace amber { -enum class ExecutionType { kExecute = 0, kPipelineCreateOnly }; - /// The executor is responsible for running the given script against an engine. class Executor { public: @@ -37,15 +35,13 @@ class Executor { /// used as the shader binary. Result Execute(Engine* engine, const Script* script, - Delegate* delegate, const ShaderMap& map, - ExecutionType executionType, - bool disable_spirv_validation = false); + Options* options); private: Result CompileShaders(const Script* script, const ShaderMap& shader_map, - bool disable_spirv_validation); + Options* options); Result ExecuteCommand(Engine* engine, Command* cmd); Verifier verifier_; diff --git a/src/executor_test.cc b/src/executor_test.cc index b8f45370f..499ef5c43 100644 --- a/src/executor_test.cc +++ b/src/executor_test.cc @@ -226,9 +226,9 @@ logicOp)"; script->GetRequiredInstanceExtensions(), script->GetRequiredDeviceExtensions()); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); const auto& features = ToStub(engine.get())->GetFeatures(); @@ -257,9 +257,9 @@ VK_KHR_variable_pointers)"; script->GetRequiredInstanceExtensions(), script->GetRequiredDeviceExtensions()); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); const auto& features = ToStub(engine.get())->GetFeatures(); @@ -288,9 +288,9 @@ depthstencil D24_UNORM_S8_UINT)"; script->GetRequiredInstanceExtensions(), script->GetRequiredDeviceExtensions()); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); const auto& features = ToStub(engine.get())->GetFeatures(); @@ -316,9 +316,9 @@ fence_timeout 12345)"; script->GetRequiredInstanceExtensions(), script->GetRequiredDeviceExtensions()); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); const auto& features = ToStub(engine.get())->GetFeatures(); @@ -350,9 +350,9 @@ fence_timeout 12345)"; script->GetRequiredInstanceExtensions(), script->GetRequiredDeviceExtensions()); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); const auto& features = ToStub(engine.get())->GetFeatures(); @@ -380,9 +380,9 @@ clear)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); EXPECT_TRUE(ToStub(engine.get())->DidClearCommand()); } @@ -400,9 +400,9 @@ clear)"; ToStub(engine.get())->FailClearCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("clear command failed", r.Error()); } @@ -419,9 +419,9 @@ clear color 244 123 123 13)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidClearColorCommand()); @@ -448,9 +448,9 @@ clear color 123 123 123 123)"; ToStub(engine.get())->FailClearColorCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("clear color command failed", r.Error()); } @@ -467,9 +467,9 @@ clear depth 24)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidClearDepthCommand()); } @@ -487,9 +487,9 @@ clear depth 24)"; ToStub(engine.get())->FailClearDepthCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("clear depth command failed", r.Error()); } @@ -506,9 +506,9 @@ clear stencil 24)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidClearStencilCommand()); } @@ -526,9 +526,9 @@ clear stencil 24)"; ToStub(engine.get())->FailClearStencilCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("clear stencil command failed", r.Error()); } @@ -545,9 +545,9 @@ draw rect 2 4 10 20)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidDrawRectCommand()); } @@ -565,9 +565,9 @@ draw rect 2 4 10 20)"; ToStub(engine.get())->FailDrawRectCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("draw rect command failed", r.Error()); } @@ -584,9 +584,9 @@ draw arrays TRIANGLE_LIST 0 0)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidDrawArraysCommand()); } @@ -604,9 +604,9 @@ draw arrays TRIANGLE_LIST 0 0)"; ToStub(engine.get())->FailDrawArraysCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("draw arrays command failed", r.Error()); } @@ -623,9 +623,9 @@ compute 2 3 4)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidComputeCommand()); } @@ -643,9 +643,9 @@ compute 2 3 4)"; ToStub(engine.get())->FailComputeCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("compute command failed", r.Error()); } @@ -662,9 +662,9 @@ vertex entrypoint main)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidEntryPointCommand()); } @@ -682,9 +682,9 @@ vertex entrypoint main)"; ToStub(engine.get())->FailEntryPointCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("entrypoint command failed", r.Error()); } @@ -701,9 +701,9 @@ patch parameter vertices 10)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidPatchParameterVerticesCommand()); } @@ -721,9 +721,9 @@ patch parameter vertices 10)"; ToStub(engine.get())->FailPatchParameterVerticesCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("patch command failed", r.Error()); } @@ -739,9 +739,9 @@ probe rect rgba 2 3 40 40 0.2 0.4 0.4 0.3)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); // ASSERT_TRUE(ToStub(engine.get())->DidProbeCommand()); } @@ -758,9 +758,9 @@ probe rect rgba 2 3 40 40 0.2 0.4 0.4 0.3)"; // ToStub(engine.get())->FailProbeCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("probe command failed", r.Error()); } @@ -777,9 +777,9 @@ ssbo 0 24)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); ASSERT_TRUE(ToStub(engine.get())->DidBufferCommand()); } @@ -797,9 +797,9 @@ ssbo 0 24)"; ToStub(engine.get())->FailBufferCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("buffer command failed", r.Error()); } @@ -815,9 +815,9 @@ probe ssbo vec3 0 2 <= 2 3 4)"; auto engine = MakeEngine(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_TRUE(r.IsSuccess()); // ASSERT_TRUE(ToStub(engine.get())->DidProbeSSBOCommand()); } @@ -834,9 +834,9 @@ probe ssbo vec3 0 2 <= 2 3 4)"; // ToStub(engine.get())->FailProbeSSBOCommand(); auto script = parser.GetScript(); + Options options; Executor ex; - Result r = ex.Execute(engine.get(), script.get(), nullptr, ShaderMap(), - ExecutionType::kExecute); + Result r = ex.Execute(engine.get(), script.get(), ShaderMap(), &options); ASSERT_FALSE(r.IsSuccess()); EXPECT_EQ("probe ssbo command failed", r.Error()); } diff --git a/src/shader_compiler.cc b/src/shader_compiler.cc index 9aca515f6..a5e98aa36 100644 --- a/src/shader_compiler.cc +++ b/src/shader_compiler.cc @@ -47,14 +47,15 @@ namespace amber { ShaderCompiler::ShaderCompiler() = default; -ShaderCompiler::ShaderCompiler(const std::string& env) : spv_env_(env) {} +ShaderCompiler::ShaderCompiler(const std::string& env, + bool disable_spirv_validation) + : spv_env_(env), disable_spirv_validation_(disable_spirv_validation) {} ShaderCompiler::~ShaderCompiler() = default; std::pair> ShaderCompiler::Compile( Pipeline::ShaderInfo* shader_info, - const ShaderMap& shader_map, - bool disable_spirv_validation) const { + const ShaderMap& shader_map) const { const auto shader = shader_info->GetShader(); auto it = shader_map.find(shader->GetName()); if (it != shader_map.end()) { @@ -143,9 +144,8 @@ std::pair> ShaderCompiler::Compile( return {Result("Invalid shader format"), results}; } - (void)disable_spirv_validation; #if AMBER_ENABLE_SPIRV_TOOLS - if (!disable_spirv_validation) { + if (!disable_spirv_validation_) { spvtools::ValidatorOptions options; if (!tools.Validate(results.data(), results.size(), options)) return {Result("Invalid shader: " + spv_errors), {}}; diff --git a/src/shader_compiler.h b/src/shader_compiler.h index b093383be..c839fca24 100644 --- a/src/shader_compiler.h +++ b/src/shader_compiler.h @@ -30,7 +30,8 @@ namespace amber { class ShaderCompiler { public: ShaderCompiler(); - explicit ShaderCompiler(const std::string& env); + explicit ShaderCompiler(const std::string& env, + bool disable_spirv_validation); ~ShaderCompiler(); /// Returns a result code and a compilation of the given shader. @@ -43,8 +44,7 @@ class ShaderCompiler { /// be invoked to produce the shader binary. std::pair> Compile( Pipeline::ShaderInfo* shader_info, - const ShaderMap& shader_map, - bool disable_spirv_validation = false) const; + const ShaderMap& shader_map) const; private: Result ParseHex(const std::string& data, std::vector* result) const; @@ -54,6 +54,7 @@ class ShaderCompiler { std::vector* result) const; std::string spv_env_; + bool disable_spirv_validation_; }; // Parses the SPIR-V environment string, and returns the corresponding From c95b7b95166836f76ea6eddd157d681d8817d229 Mon Sep 17 00:00:00 2001 From: Paul Thomson Date: Fri, 2 Aug 2019 17:06:46 +0100 Subject: [PATCH 3/4] Address comments. Braces. Initialize field. Remove explicit. --- src/executor.cc | 3 ++- src/shader_compiler.cc | 2 +- src/shader_compiler.h | 3 +-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/executor.cc b/src/executor.cc index 2bd25f733..f3c829f8b 100644 --- a/src/executor.cc +++ b/src/executor.cc @@ -82,9 +82,10 @@ Result Executor::Execute(Engine* engine, // Process Commands for (const auto& cmd : script->GetCommands()) { - if (options->delegate && options->delegate->LogExecuteCalls()) + if (options->delegate && options->delegate->LogExecuteCalls()) { options->delegate->Log(std::to_string(cmd->GetLine()) + ": " + cmd->ToString()); + } Result r = ExecuteCommand(engine, cmd.get()); if (!r.IsSuccess()) diff --git a/src/shader_compiler.cc b/src/shader_compiler.cc index a5e98aa36..03817cbb2 100644 --- a/src/shader_compiler.cc +++ b/src/shader_compiler.cc @@ -45,7 +45,7 @@ namespace amber { -ShaderCompiler::ShaderCompiler() = default; +ShaderCompiler::ShaderCompiler() : disable_spirv_validation_(false) {} ShaderCompiler::ShaderCompiler(const std::string& env, bool disable_spirv_validation) diff --git a/src/shader_compiler.h b/src/shader_compiler.h index c839fca24..c070b9c7a 100644 --- a/src/shader_compiler.h +++ b/src/shader_compiler.h @@ -30,8 +30,7 @@ namespace amber { class ShaderCompiler { public: ShaderCompiler(); - explicit ShaderCompiler(const std::string& env, - bool disable_spirv_validation); + ShaderCompiler(const std::string& env, bool disable_spirv_validation); ~ShaderCompiler(); /// Returns a result code and a compilation of the given shader. From d060925876c0ad2ed6ffcc7137734c6a5b0a0fb9 Mon Sep 17 00:00:00 2001 From: Paul Thomson Date: Fri, 2 Aug 2019 18:27:50 +0100 Subject: [PATCH 4/4] Use in-class initialization --- src/shader_compiler.cc | 2 +- src/shader_compiler.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shader_compiler.cc b/src/shader_compiler.cc index 03817cbb2..a5e98aa36 100644 --- a/src/shader_compiler.cc +++ b/src/shader_compiler.cc @@ -45,7 +45,7 @@ namespace amber { -ShaderCompiler::ShaderCompiler() : disable_spirv_validation_(false) {} +ShaderCompiler::ShaderCompiler() = default; ShaderCompiler::ShaderCompiler(const std::string& env, bool disable_spirv_validation) diff --git a/src/shader_compiler.h b/src/shader_compiler.h index c070b9c7a..bf5fc49a2 100644 --- a/src/shader_compiler.h +++ b/src/shader_compiler.h @@ -53,7 +53,7 @@ class ShaderCompiler { std::vector* result) const; std::string spv_env_; - bool disable_spirv_validation_; + bool disable_spirv_validation_ = false; }; // Parses the SPIR-V environment string, and returns the corresponding