Re: [Mesa-dev] [PATCH 07/15] clover/spirv: Add functions for parsing arguments, linking programs, etc.
> > + throw build_error(); \
> > + }
> > +
> > + switch (dim) {
> > + case SpvDim2D:
> > + APPEND_DIM(2d)
>
> I was shortly confused about whether this could fall through. It would
> be cleaner to do this with a single if/else ladder like
> clover/llvm/codegen/common.cpp. You would only be able to output a
> single error message though (e.g. "Unsupported qualifiers $DIM $ACCESS
> on image $ID"), but that seems good enough to me (do we even need an
> error message here instead of an assertion failure?).
I got confused too when reading it; I changed it to an if/else ladder.
I kept the error message as we are currently not handling all valid
combinations in clover right now (like read-write access), so we shouldn’t make
it an assert just yet.
> > + size_t i = 5u; // Skip header
>
> It would be nice if there was a define for this, but okay...
I added a SPIRV_HEADER_WORD_SIZE define.
> > +#define GET_OPERAND(type, operand_id) get(source.data(), i +
> > operand_id)
> > +
> > + while (i < length) {
>
> You could declare a 'const auto inst = [i * sizeof(uint32_t)]'
> pointer here, then do 'get(inst, j)' instead of 'GET_OPERAND(T, j)',
> and get rid of the macro definition.
Changed by Karol.
> > +// We only care about constants that represents the size of
> > arrays.
>
> "represent"
Fixed by Karol.
> > + case SpvOpTypeArray: {
> > +const auto id = GET_OPERAND(SpvId, 1);
> > +const auto type_id = GET_OPERAND(SpvId, 2);
> > +const auto types_iter = types.find(type_id);
> > +if (types_iter == types.end())
> > + break;
> > +
>
> Shouldn't this throw an error condition? There are many similar cases
> below.
No; I added a comment explaining why.
The idea behind it, is that
1. the binary has already been validated and
2. we only parse the types that are valid for entry points, so if we don’t find
the type corresponding to an ID in the list, that means this type is not
used by an entry point and therefore we don’t care about it.
> Hmm, that's a lot of additional errors. Maybe you could promote the
> fail() helper from llvm/util.hpp to core/compiler.hpp, and use it
> everywhere you need to output an error here.
Good idea.
> > +
> > + case SpvOpTypeStruct: {
> > +const auto id = GET_OPERAND(SpvId, 1);
> > +const bool is_packed = packed_structures.count(id);
> > +
> > +unsigned struct_size = 0u;
> > +unsigned max_elem_size = 0u;
>
> This seems really meant to keep track of the maximum element alignment.
> Maybe call it 'max_elem_align'? Would likely make sense to initialize
> it to 1.
Changed to `struct_align` by Karol.
> > +for (unsigned j = 2u; j < num_operands; ++j) {
> > + const auto type_id = GET_OPERAND(SpvId, j);
> > + const auto types_iter = types.find(type_id);
> > + if (types_iter == types.end())
> > + break;
> > +
> > + const auto alignment = is_packed ? 1u
> > +:
> > types_iter->second.target_align;
> > + const auto padding = (-struct_size) & (alignment - 1u);
> > + struct_size += padding + types_iter->second.target_size;
> > + max_elem_size = std::max(max_elem_size,
> > +types_iter->second.target_align);
>
> If you make 'max_elem_size = std::max(max_elem_size, alignment)' you can
> get rid of the is_packed ternary below.
Changed by Karol.
> > +}
> > +if (!is_packed)
>
> Conditional is unnecessary if you take my suggestions above.
Changed by Karol.
> > + struct_size += (-struct_size) & (max_elem_size - 1u);
> > +
> > +types[id] = { module::argument::scalar, struct_size,
> > struct_size,
> > + is_packed ? 1u
> > +: max_elem_size,
> > module::argument::zero_ext
> > +};
>
> Place the closing curly bracket on the last line of the aggregate
> initializer, here and below.
Changed by Karol.
> > + if (opcode != SpvOpCapability)
> > +break;
> > +
>
> Isn't this going to miss most of the program if there is e.g. a
> SpvOpSource before the first capability?
This would be invalid. Maybe I should add a comment that valid SPIR-V is
expected?
> > + if (opcode == SpvOpCapability) {
> > +i += num_operands;
> > +continue;
> > + }
> > + if (opcode != SpvOpExtension)
> > +break;
>
> Similar issue here.
Same comment here.
> > +module
> > +clover::spirv::process_program(const std::vector ,
> > + const device , bool validate,
> > + std::string _log) {
>
> We should call this function "compile_program()" for consistency with
> its LLVM IR
Re: [Mesa-dev] [PATCH 07/15] clover/spirv: Add functions for parsing arguments, linking programs, etc.
Karol Herbst writes:
> From: Pierre Moreau
>
> v2 (Karol Herbst):
> silence warnings about unhandled enum values
> ---
> .../clover/spirv/invocation.cpp | 598 ++
> .../clover/spirv/invocation.hpp | 12 +
> 2 files changed, 610 insertions(+)
>
> diff --git a/src/gallium/state_trackers/clover/spirv/invocation.cpp
> b/src/gallium/state_trackers/clover/spirv/invocation.cpp
> index b874f2f061c..62886e77495 100644
> --- a/src/gallium/state_trackers/clover/spirv/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/spirv/invocation.cpp
> @@ -22,10 +22,24 @@
>
> #include "invocation.hpp"
>
> +#include
You don't seem to be using this include.
> +#include
> +#include
> +#include
> +#include
> +
> #ifdef CLOVER_ALLOW_SPIRV
> #include
> +#include
> #endif
>
> +#include "core/error.hpp"
> +#include "core/platform.hpp"
> +#include "invocation.hpp"
> +#include "llvm/util.hpp"
> +#include "pipe/p_state.h"
> +#include "util/algorithm.hpp"
> +#include "util/functional.hpp"
> #include "util/u_math.h"
>
> #include "compiler/spirv/spirv.h"
> @@ -34,6 +48,472 @@ using namespace clover;
>
> namespace {
>
> + template
> + T get(const char *source, size_t index) {
> + const uint32_t *word_ptr = reinterpret_cast(source);
> + return static_cast(word_ptr[index]);
> + }
> +
> + enum module::argument::type
> + convertStorageClass(SpvStorageClass storage_class, std::string ) {
Use underscores instead of camel case here and below.
> + switch (storage_class) {
> + case SpvStorageClassFunction:
> + return module::argument::scalar;
> + case SpvStorageClassUniformConstant:
> + return module::argument::constant;
> + case SpvStorageClassWorkgroup:
> + return module::argument::local;
> + case SpvStorageClassCrossWorkgroup:
> + return module::argument::global;
> + default:
> + err += "Invalid storage type " + std::to_string(storage_class) +
> "\n";
> + throw build_error();
> + }
> + }
> +
> + enum module::argument::type
> + convertImageType(SpvId id, SpvDim dim, SpvAccessQualifier access,
> +std::string ) {
> +#define APPEND_DIM(d) \
> + switch(access) { \
> + case SpvAccessQualifierReadOnly: \
> + return module::argument::image##d##_rd; \
> + case SpvAccessQualifierWriteOnly: \
> + return module::argument::image##d##_wr; \
> + default: \
> + err += "Unsupported access qualifier " #d " for image " + \
> +std::to_string(id); \
Error message seems broken, #d is the image dimensionality, not the
access qualifier.
> + throw build_error(); \
> + }
> +
> + switch (dim) {
> + case SpvDim2D:
> + APPEND_DIM(2d)
I was shortly confused about whether this could fall through. It would
be cleaner to do this with a single if/else ladder like
clover/llvm/codegen/common.cpp. You would only be able to output a
single error message though (e.g. "Unsupported qualifiers $DIM $ACCESS
on image $ID"), but that seems good enough to me (do we even need an
error message here instead of an assertion failure?).
> + case SpvDim3D:
> + APPEND_DIM(3d)
> + default:
> + err += "Unsupported dimension " + std::to_string(dim) + " for image
> " +
> +std::to_string(id);
> + throw build_error();
> + }
> +
> +#undef APPEND_DIM
> + }
> +
> + module::section
> + make_text_section(const std::vector ,
> + enum module::section::type section_type) {
> + const pipe_llvm_program_header header { uint32_t(code.size()) };
> + module::section text { 0, section_type, header.num_bytes, {} };
> +
> + text.data.insert(text.data.end(), reinterpret_cast *>(),
> + reinterpret_cast() +
> sizeof(header));
> + text.data.insert(text.data.end(), code.begin(), code.end());
> +
> + return text;
> + }
> +
> + module
> + create_module_from_spirv(const std::vector ,
> +size_t pointer_byte_size,
> +std::string ) {
> + const size_t length = source.size() / sizeof(uint32_t);
> + size_t i = 5u; // Skip header
It would be nice if there was a define for this, but okay...
> +
> + std::string kernel_name;
> + size_t kernel_nb = 0u;
> + std::vector args;
> +
> + module m;
> +
> + std::unordered_map kernels;
> + std::unordered_map types;
> + std::unordered_map pointer_types;
> + std::unordered_map constants;
> + std::unordered_set packed_structures;
> + std::unordered_map>
> + func_param_attr_map;
> +
> +#define GET_OPERAND(type, operand_id) get(source.data(), i +
> operand_id)
> +
> + while (i < length) {
You could declare a 'const auto inst = [i * sizeof(uint32_t)]'
pointer here, then do 'get(inst, j)' instead of 'GET_OPERAND(T, j)',
and get rid of
[Mesa-dev] [PATCH 07/15] clover/spirv: Add functions for parsing arguments, linking programs, etc.
From: Pierre Moreau
v2 (Karol Herbst):
silence warnings about unhandled enum values
---
.../clover/spirv/invocation.cpp | 598 ++
.../clover/spirv/invocation.hpp | 12 +
2 files changed, 610 insertions(+)
diff --git a/src/gallium/state_trackers/clover/spirv/invocation.cpp
b/src/gallium/state_trackers/clover/spirv/invocation.cpp
index b874f2f061c..62886e77495 100644
--- a/src/gallium/state_trackers/clover/spirv/invocation.cpp
+++ b/src/gallium/state_trackers/clover/spirv/invocation.cpp
@@ -22,10 +22,24 @@
#include "invocation.hpp"
+#include
+#include
+#include
+#include
+#include
+
#ifdef CLOVER_ALLOW_SPIRV
#include
+#include
#endif
+#include "core/error.hpp"
+#include "core/platform.hpp"
+#include "invocation.hpp"
+#include "llvm/util.hpp"
+#include "pipe/p_state.h"
+#include "util/algorithm.hpp"
+#include "util/functional.hpp"
#include "util/u_math.h"
#include "compiler/spirv/spirv.h"
@@ -34,6 +48,472 @@ using namespace clover;
namespace {
+ template
+ T get(const char *source, size_t index) {
+ const uint32_t *word_ptr = reinterpret_cast(source);
+ return static_cast(word_ptr[index]);
+ }
+
+ enum module::argument::type
+ convertStorageClass(SpvStorageClass storage_class, std::string ) {
+ switch (storage_class) {
+ case SpvStorageClassFunction:
+ return module::argument::scalar;
+ case SpvStorageClassUniformConstant:
+ return module::argument::constant;
+ case SpvStorageClassWorkgroup:
+ return module::argument::local;
+ case SpvStorageClassCrossWorkgroup:
+ return module::argument::global;
+ default:
+ err += "Invalid storage type " + std::to_string(storage_class) + "\n";
+ throw build_error();
+ }
+ }
+
+ enum module::argument::type
+ convertImageType(SpvId id, SpvDim dim, SpvAccessQualifier access,
+std::string ) {
+#define APPEND_DIM(d) \
+ switch(access) { \
+ case SpvAccessQualifierReadOnly: \
+ return module::argument::image##d##_rd; \
+ case SpvAccessQualifierWriteOnly: \
+ return module::argument::image##d##_wr; \
+ default: \
+ err += "Unsupported access qualifier " #d " for image " + \
+std::to_string(id); \
+ throw build_error(); \
+ }
+
+ switch (dim) {
+ case SpvDim2D:
+ APPEND_DIM(2d)
+ case SpvDim3D:
+ APPEND_DIM(3d)
+ default:
+ err += "Unsupported dimension " + std::to_string(dim) + " for image "
+
+std::to_string(id);
+ throw build_error();
+ }
+
+#undef APPEND_DIM
+ }
+
+ module::section
+ make_text_section(const std::vector ,
+ enum module::section::type section_type) {
+ const pipe_llvm_program_header header { uint32_t(code.size()) };
+ module::section text { 0, section_type, header.num_bytes, {} };
+
+ text.data.insert(text.data.end(), reinterpret_cast(),
+ reinterpret_cast() +
sizeof(header));
+ text.data.insert(text.data.end(), code.begin(), code.end());
+
+ return text;
+ }
+
+ module
+ create_module_from_spirv(const std::vector ,
+size_t pointer_byte_size,
+std::string ) {
+ const size_t length = source.size() / sizeof(uint32_t);
+ size_t i = 5u; // Skip header
+
+ std::string kernel_name;
+ size_t kernel_nb = 0u;
+ std::vector args;
+
+ module m;
+
+ std::unordered_map kernels;
+ std::unordered_map types;
+ std::unordered_map pointer_types;
+ std::unordered_map constants;
+ std::unordered_set packed_structures;
+ std::unordered_map>
+ func_param_attr_map;
+
+#define GET_OPERAND(type, operand_id) get(source.data(), i + operand_id)
+
+ while (i < length) {
+ const auto desc_word = get(source.data(), i);
+ const auto opcode = static_cast(desc_word & SpvOpCodeMask);
+ const unsigned int num_operands = desc_word >> SpvWordCountShift;
+
+ switch (opcode) {
+ case SpvOpEntryPoint:
+if (GET_OPERAND(SpvExecutionModel, 1) == SpvExecutionModelKernel)
+ kernels.emplace(GET_OPERAND(SpvId, 2),
+ source.data() + (i + 3u) * sizeof(uint32_t));
+break;
+
+ case SpvOpDecorate: {
+const auto id = GET_OPERAND(SpvId, 1);
+const auto decoration = GET_OPERAND(SpvDecoration, 2);
+if (decoration == SpvDecorationCPacked)
+ packed_structures.emplace(id);
+else if (decoration == SpvDecorationFuncParamAttr)
+
func_param_attr_map[id].push_back(GET_OPERAND(SpvFunctionParameterAttribute,
3u));
+break;
+ }
+
+ case SpvOpGroupDecorate: {
+const auto group_id = GET_OPERAND(SpvId, 1);
+if
