From 27c8c2fcba747f7f5bcf1988eed31fcb6c57afa1 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Sun, 8 Jul 2018 19:05:13 +0100 Subject: [PATCH 1/6] Fixed SPIR-V codegen incorrectly not declaring the interface for the entry point --- src/codegen/spirv/codegen_spirv.cc | 2 +- src/codegen/spirv/ir_builder.cc | 21 ++++++++++++++++++--- src/codegen/spirv/ir_builder.h | 2 +- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/codegen/spirv/codegen_spirv.cc b/src/codegen/spirv/codegen_spirv.cc index b6582ec6c0d8..6e97cdf77c84 100644 --- a/src/codegen/spirv/codegen_spirv.cc +++ b/src/codegen/spirv/codegen_spirv.cc @@ -35,7 +35,7 @@ std::vector CodeGenSPIRV::BuildFunction(const LoweredFunc& f) { pod_args.push_back(arg); } } - spirv::Value func_ptr = builder_->DeclareKenrelFunction(f->name); + spirv::Value func_ptr = builder_->DeclareKernelFunction(f->name, true, true); builder_->StartFunction(func_ptr); // All the POD arguments are passed in through PushConstant diff --git a/src/codegen/spirv/ir_builder.cc b/src/codegen/spirv/ir_builder.cc index f3e044596e5c..42e81dadb8de 100644 --- a/src/codegen/spirv/ir_builder.cc +++ b/src/codegen/spirv/ir_builder.cc @@ -222,11 +222,26 @@ Value IRBuilder::GetPushConstant( return this->MakeValue(spv::OpLoad, v_type, ptr); } -Value IRBuilder::DeclareKenrelFunction(const std::string& name) { +Value IRBuilder::DeclareKernelFunction(const std::string& name, + bool uses_workgroup_id, + bool uses_local_id) { + if (uses_workgroup_id) { + GetWorkgroupID(0); // Ensure workgroup_id_ is valid + } + if (uses_local_id) { + GetLocalID(0); // Ensure local_id_ is valid + } + Value val = NewValue(t_void_func_, kFunction); ib_.Begin(spv::OpEntryPoint) - .AddSeq(spv::ExecutionModelGLCompute, val, name) - .Commit(&entry_); + .AddSeq(spv::ExecutionModelGLCompute, val, name); + if (uses_workgroup_id) { + ib_.Add(workgroup_id_); + } + if (uses_local_id) { + ib_.Add(local_id_); + } + ib_.Commit(&entry_); return val; } diff --git a/src/codegen/spirv/ir_builder.h b/src/codegen/spirv/ir_builder.h index fc2d5bc68b6f..52f2cc128b91 100644 --- a/src/codegen/spirv/ir_builder.h +++ b/src/codegen/spirv/ir_builder.h @@ -489,7 +489,7 @@ class IRBuilder { * \param name Name of the entry point. * \return The created function ID. */ - Value DeclareKenrelFunction(const std::string& name); + Value DeclareKernelFunction(const std::string& name, bool uses_workgroup_id = false, bool uses_local_id = false); /*! * \brief Start function scope. * \param func function to be started. From 168fea78e1e0194987a0c9b9e0961ba0e2bdd2f7 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Sun, 8 Jul 2018 19:29:03 +0100 Subject: [PATCH 2/6] fix lint --- src/codegen/spirv/ir_builder.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/codegen/spirv/ir_builder.h b/src/codegen/spirv/ir_builder.h index 52f2cc128b91..ad1887712f5f 100644 --- a/src/codegen/spirv/ir_builder.h +++ b/src/codegen/spirv/ir_builder.h @@ -487,9 +487,13 @@ class IRBuilder { /*! * \brief Declare a kernel function * \param name Name of the entry point. + * \param uses_workgroup_id Whether the function will use the workgroup id + * \param uses_local_id Whether the function will use the local invocation id * \return The created function ID. */ - Value DeclareKernelFunction(const std::string& name, bool uses_workgroup_id = false, bool uses_local_id = false); + Value DeclareKernelFunction(const std::string& name, + bool uses_workgroup_id = false, + bool uses_local_id = false); /*! * \brief Start function scope. * \param func function to be started. From 195d29e614a1bda93fcfa47e27da815468d8f396 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Sun, 8 Jul 2018 19:30:43 +0100 Subject: [PATCH 3/6] fix lint --- src/codegen/spirv/ir_builder.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/codegen/spirv/ir_builder.cc b/src/codegen/spirv/ir_builder.cc index 42e81dadb8de..5362a0a4c04f 100644 --- a/src/codegen/spirv/ir_builder.cc +++ b/src/codegen/spirv/ir_builder.cc @@ -226,10 +226,10 @@ Value IRBuilder::DeclareKernelFunction(const std::string& name, bool uses_workgroup_id, bool uses_local_id) { if (uses_workgroup_id) { - GetWorkgroupID(0); // Ensure workgroup_id_ is valid + GetWorkgroupID(0); // Ensure workgroup_id_ is valid } if (uses_local_id) { - GetLocalID(0); // Ensure local_id_ is valid + GetLocalID(0); // Ensure local_id_ is valid } Value val = NewValue(t_void_func_, kFunction); From f62452e1820ca4f9a9e005ef28af6e6553120cfb Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Mon, 9 Jul 2018 19:24:20 +0100 Subject: [PATCH 4/6] Refactored SPIR-V entry point fix --- src/codegen/spirv/codegen_spirv.cc | 4 +++- src/codegen/spirv/ir_builder.cc | 28 ++++++++++++++-------------- src/codegen/spirv/ir_builder.h | 17 ++++++++++------- 3 files changed, 27 insertions(+), 22 deletions(-) diff --git a/src/codegen/spirv/codegen_spirv.cc b/src/codegen/spirv/codegen_spirv.cc index 6e97cdf77c84..2c71d94eaba1 100644 --- a/src/codegen/spirv/codegen_spirv.cc +++ b/src/codegen/spirv/codegen_spirv.cc @@ -35,7 +35,7 @@ std::vector CodeGenSPIRV::BuildFunction(const LoweredFunc& f) { pod_args.push_back(arg); } } - spirv::Value func_ptr = builder_->DeclareKernelFunction(f->name, true, true); + spirv::Value func_ptr = builder_->NewFunction(); builder_->StartFunction(func_ptr); // All the POD arguments are passed in through PushConstant @@ -56,6 +56,8 @@ std::vector CodeGenSPIRV::BuildFunction(const LoweredFunc& f) { builder_->MakeInst(spv::OpReturn); builder_->MakeInst(spv::OpFunctionEnd); + builder_->CommitKernelFunction(func_ptr, f->name); + return builder_->Finalize(); } diff --git a/src/codegen/spirv/ir_builder.cc b/src/codegen/spirv/ir_builder.cc index 5362a0a4c04f..e88069ebba91 100644 --- a/src/codegen/spirv/ir_builder.cc +++ b/src/codegen/spirv/ir_builder.cc @@ -222,27 +222,27 @@ Value IRBuilder::GetPushConstant( return this->MakeValue(spv::OpLoad, v_type, ptr); } -Value IRBuilder::DeclareKernelFunction(const std::string& name, - bool uses_workgroup_id, - bool uses_local_id) { - if (uses_workgroup_id) { - GetWorkgroupID(0); // Ensure workgroup_id_ is valid - } - if (uses_local_id) { - GetLocalID(0); // Ensure local_id_ is valid - } +Value IRBuilder::NewFunction() { + return NewValue(t_void_func_, kFunction); +} + +void IRBuilder::CommitKernelFunction(const Value& func_id, const std::string& name) { + //if (uses_workgroup_id) { + // GetWorkgroupID(0); // Ensure workgroup_id_ is valid + //} + //if (uses_local_id) { + // GetLocalID(0); // Ensure local_id_ is valid + //} - Value val = NewValue(t_void_func_, kFunction); ib_.Begin(spv::OpEntryPoint) - .AddSeq(spv::ExecutionModelGLCompute, val, name); - if (uses_workgroup_id) { + .AddSeq(spv::ExecutionModelGLCompute, func_id, name); + if (workgroup_id_.id != 0) { ib_.Add(workgroup_id_); } - if (uses_local_id) { + if (local_id_.id != 0) { ib_.Add(local_id_); } ib_.Commit(&entry_); - return val; } void IRBuilder::StartFunction(const Value& func) { diff --git a/src/codegen/spirv/ir_builder.h b/src/codegen/spirv/ir_builder.h index ad1887712f5f..75f07ae5d212 100644 --- a/src/codegen/spirv/ir_builder.h +++ b/src/codegen/spirv/ir_builder.h @@ -485,15 +485,18 @@ class IRBuilder { */ Value GetPushConstant(Value ptr_push_const, const SType& v_type, uint32_t index); /*! - * \brief Declare a kernel function - * \param name Name of the entry point. - * \param uses_workgroup_id Whether the function will use the workgroup id - * \param uses_local_id Whether the function will use the local invocation id + * \brief Declare a new function * \return The created function ID. */ - Value DeclareKernelFunction(const std::string& name, - bool uses_workgroup_id = false, - bool uses_local_id = false); + Value NewFunction(); + /*! + * \brief Declare the entry point for a kernel function. This should be + * invoked after building the function so the builder is aware of which + * variables to declare as part of the function's interface. + * \param func_id The id of the previously declared function. + * \param name Name of the entry point. + */ + void CommitKernelFunction(const Value& func_id, const std::string& name); /*! * \brief Start function scope. * \param func function to be started. From eec2c422940183160df59e7d77e3e7c7b4a09ef1 Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Mon, 9 Jul 2018 19:26:01 +0100 Subject: [PATCH 5/6] Removed commented code --- src/codegen/spirv/ir_builder.cc | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/codegen/spirv/ir_builder.cc b/src/codegen/spirv/ir_builder.cc index e88069ebba91..12a9f138bb72 100644 --- a/src/codegen/spirv/ir_builder.cc +++ b/src/codegen/spirv/ir_builder.cc @@ -227,13 +227,6 @@ Value IRBuilder::NewFunction() { } void IRBuilder::CommitKernelFunction(const Value& func_id, const std::string& name) { - //if (uses_workgroup_id) { - // GetWorkgroupID(0); // Ensure workgroup_id_ is valid - //} - //if (uses_local_id) { - // GetLocalID(0); // Ensure local_id_ is valid - //} - ib_.Begin(spv::OpEntryPoint) .AddSeq(spv::ExecutionModelGLCompute, func_id, name); if (workgroup_id_.id != 0) { From 6a0c31bc51635fc9d3f44a4b84ebb20f6e2dc66f Mon Sep 17 00:00:00 2001 From: Alex Weaver Date: Mon, 9 Jul 2018 19:27:56 +0100 Subject: [PATCH 6/6] Added check for function arg --- src/codegen/spirv/ir_builder.cc | 5 +++-- src/codegen/spirv/ir_builder.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/codegen/spirv/ir_builder.cc b/src/codegen/spirv/ir_builder.cc index 12a9f138bb72..eb7a67228e60 100644 --- a/src/codegen/spirv/ir_builder.cc +++ b/src/codegen/spirv/ir_builder.cc @@ -226,9 +226,10 @@ Value IRBuilder::NewFunction() { return NewValue(t_void_func_, kFunction); } -void IRBuilder::CommitKernelFunction(const Value& func_id, const std::string& name) { +void IRBuilder::CommitKernelFunction(const Value& func, const std::string& name) { + CHECK_EQ(func.flag, kFunction); ib_.Begin(spv::OpEntryPoint) - .AddSeq(spv::ExecutionModelGLCompute, func_id, name); + .AddSeq(spv::ExecutionModelGLCompute, func, name); if (workgroup_id_.id != 0) { ib_.Add(workgroup_id_); } diff --git a/src/codegen/spirv/ir_builder.h b/src/codegen/spirv/ir_builder.h index 75f07ae5d212..7c7afabdf566 100644 --- a/src/codegen/spirv/ir_builder.h +++ b/src/codegen/spirv/ir_builder.h @@ -493,10 +493,10 @@ class IRBuilder { * \brief Declare the entry point for a kernel function. This should be * invoked after building the function so the builder is aware of which * variables to declare as part of the function's interface. - * \param func_id The id of the previously declared function. + * \param func The previously declared function. * \param name Name of the entry point. */ - void CommitKernelFunction(const Value& func_id, const std::string& name); + void CommitKernelFunction(const Value& func, const std::string& name); /*! * \brief Start function scope. * \param func function to be started.