spv_parsed_instruction_t cleanup #21
Conversation
Add members: - words: a pointer to an array of words in the instruction, in host native endianness. - num_words: sizes the words member Remove member: - offset This simplifies clients of spvBinaryParse, because they don't have to handle endianness translation. Also, it makes the binary parse API more composable, allowing for easy chaining of binary parse clients. A binary parse client is handed the array of words directly instead of having to reference some external array of all the words in the SPIR-V binary. It also allows a binary parse client to mutate the instruction stream before handing off to a downstream consumer. TODO(dneto): Still need to write the unit tests for spvBinaryParse Fixes: KhronosGroup#1
| spv_result_t parseOperand(spv_parsed_instruction_t* inst, | ||
| // Parses an instruction operand with the given type, for an instruction | ||
| // starting at inst_offset words into the SPIR-V binary. Appends, to the | ||
| // words parameter, the endian-translated words for the operand, but only if |
There was a problem hiding this comment.
Isn't there a simpler interface for this? I'm finding it hard to follow.
There was a problem hiding this comment.
This becomes a fairly complicated interface...
There was a problem hiding this comment.
The tipping point on the complexity of this is caused by the fact that the method behaves differently based on whether endian conversion must be done or not. If no endian conversion is required, then the endian_converted_inst_words parameter is ignored. Life would be simpler if we didn't mind copying the underlying data in that case. We save on allocations and copying time in that very common case. I think it's worth keeping that optimization.
Maybe I worded it poorly. How about replacing the sentence from "Appends....from host" with:
If the SPIR-V binary is the same endianness as the host, then the endian_converted_inst_words parameter is ignored. Otherwise, this method appends the words for this operand, converted to host native endianness, to the end of endian_converted_inst_words.
There was a problem hiding this comment.
+1. I like the later version.
There was a problem hiding this comment.
changed to new version
|
+2 |
|
Rebased, squashed, and pushed onto master |
Add members:
in host native endianness.
Remove member:
This simplifies clients of spvBinaryParse, because they don't
have to handle endianness translation.
Also, it makes the binary parse API more composable, allowing
for easy chaining of binary parse clients. A binary parse client
is handed the array of words directly instead of having to reference
some external array of all the words in the SPIR-V binary. It also
allows a binary parse client to mutate the instruction stream before
handing off to a downstream consumer.
TODO(dneto): Still need to write the unit tests for spvBinaryParse
Fixes: #1