[Mesa-dev] [PATCH 07/15] clover/spirv: Add functions for parsing arguments, linking programs, etc.
Pierre Moreau
dev at pmoreau.org
Wed May 22 23:31:29 UTC 2019
> > + 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<type>(source.data(), i + operand_id)
> > +
> > + while (i < length) {
>
> You could declare a 'const auto inst = &source[i * sizeof(uint32_t)]'
> pointer here, then do 'get<T>(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<char> &binary,
> > + const device &dev, bool validate,
> > + std::string &r_log) {
>
> We should call this function "compile_program()" for consistency with
> its LLVM IR counterpart. Also the 'validate' boolean seems to be always
> true.
Renamed to `compile_program()`. The boolean is used in the next series, since
the validation is performed in `clCreateProgramWithIL()` so there is no point
validating it again when calling `compile_program()`.
> > + std::vector<char> source = spirv_to_cpu(binary);
> > +
> > + if (validate && !is_valid_spirv(
> > + reinterpret_cast<const uint32_t *>(source.data()),
> > + source.size() / 4u, dev.device_version(),
>
> Is there any reason not to pass a 'const std::vector<char> &' to
> is_valid_spirv instead of a separate uint32_t pointer and size?
I might have been matching what was used by SPIRV-Tools; changed the function
to take a vector reference instead.
> > #ifdef CLOVER_ALLOW_SPIRV
>
> Shouldn't most of the code above be compiled conditionally based on this
> define?
Good point: I moved the #ifdef up in the file, so now all functions are only
compiled if it is set.
> > + if (!options.empty()) {
> > + r_log += "SPIR-V linker: Ignoring the following link options: ";
> > + for (const auto &opt : options)
> > + r_log += "'" + opt + "' ";
> > + }
> > +
>
> This seems like a really inefficient way to check for unused options.
> You could just loop over the options vector non-destructively and print
> out the ones you don't know.
Changed to
```
bool create_library = false;
std::string ignored_options;
for (const std::string &option : options) {
if (option == "-create-library") {
create_library = true;
} else {
ignored_options += "'" + option + "' ";
}
}
if (!ignored_options.empty()) {
r_log += "Ignoring the following link options: " + ignored_options
+ "\n";
}
```
> > + } catch (const std::out_of_range &e) {
> > + // We should never reach this as validate_link_devices already checked
> > + // that a binary was present.
> > + assert(false);
> > + }
> > +
>
> Catching an exception in order to assert(0) doesn't seem useful. How
> about replacing the whole block with:
>
> | const auto &msec = find([&](const module::section &sec) {
> | return sec.type == module::section::text_intermediate ||
> | sec.type == module::section::text_library;
> | }, mod.secs);
Done
> > + m.secs.emplace_back(make_text_section({
> > + reinterpret_cast<char *>(linked_binary.data()),
> > + reinterpret_cast<char *>(linked_binary.data()) +
> > + linked_binary.size() * sizeof(uint32_t) }, section_type));
>
> Simplify 'reinterpret_cast<char *>(linked_binary.data()) +
> linked_binary.size() * sizeof(uint32_t)' to 'reinterpret_cast<char
> *>(linked_binary.data() + linked_binary.size())'.
Changed by Karol.
Thank you for the review; the updated code was pushed to the MR. Would you like
a new series on the ML or will you check on the MR?
Thanks!
Pierre
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190523/ba138dad/attachment.sig>
More information about the mesa-dev
mailing list