-
Notifications
You must be signed in to change notification settings - Fork 564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes for various endianness issues #5302
base: main
Are you sure you want to change the base?
Changes from all commits
4826b6e
39a63f8
e600563
3ccc744
74df3cf
458eb86
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,7 +173,7 @@ class Parser { | |
// Returns the endian-corrected word at the given position. | ||
uint32_t peekAt(size_t index) const { | ||
assert(index < _.num_words); | ||
return spvFixWord(_.words[index], _.endian); | ||
return _.native_words[index]; | ||
} | ||
|
||
// Data members | ||
|
@@ -218,6 +218,7 @@ class Parser { | |
// Is the SPIR-V binary in a different endianness from the host native | ||
// endianness? | ||
bool requires_endian_conversion; | ||
std::unique_ptr<uint32_t[]> native_words; | ||
|
||
// Maps a result ID to its type ID. By convention: | ||
// - a result ID that is a type definition maps to itself. | ||
|
@@ -262,6 +263,9 @@ spv_result_t Parser::parseModule() { | |
<< _.words[0] << "'."; | ||
} | ||
_.requires_endian_conversion = !spvIsHostEndian(_.endian); | ||
_.native_words = std::make_unique<uint32_t[]>(_.num_words); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's try to fix the logic without doubling copying the whole module, for everybody in all situations. |
||
for (size_t i = 0; i < _.num_words; i++) | ||
_.native_words[i] = _.requires_endian_conversion ? spvFixWord(_.words[i], _.endian) : _.words[i]; | ||
|
||
// Process the header. | ||
spv_header_t header; | ||
|
@@ -440,10 +444,6 @@ spv_result_t Parser::parseOperand(size_t inst_offset, | |
|
||
const uint32_t word = peek(); | ||
|
||
// Do the words in this operand have to be converted to native endianness? | ||
// True for all but literal strings. | ||
bool convert_operand_endianness = true; | ||
|
||
switch (type) { | ||
case SPV_OPERAND_TYPE_TYPE_ID: | ||
if (!word) | ||
|
@@ -590,7 +590,7 @@ spv_result_t Parser::parseOperand(size_t inst_offset, | |
case SPV_OPERAND_TYPE_OPTIONAL_LITERAL_STRING: { | ||
const size_t max_words = _.num_words - _.word_index; | ||
std::string string = | ||
spvtools::utils::MakeString(_.words + _.word_index, max_words, false); | ||
spvtools::utils::MakeString(_.native_words.get() + _.word_index, max_words, false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this change is correct. The SPEC says:
How do you know that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the spec confusingly says "little-endian" it actually calls for packing bytes into 4-byte words by shifting them, which is what We therefore want all the words to be in the native order because |
||
|
||
if (string.length() == max_words * 4) | ||
return exhaustedInputDiagnostic(inst_offset, opcode, type); | ||
|
@@ -752,17 +752,8 @@ spv_result_t Parser::parseOperand(size_t inst_offset, | |
|
||
if (_.requires_endian_conversion) { | ||
// Copy instruction words. Translate to native endianness as needed. | ||
if (convert_operand_endianness) { | ||
const spv_endianness_t endianness = _.endian; | ||
std::transform(_.words + _.word_index, _.words + index_after_operand, | ||
std::back_inserter(*words), | ||
[endianness](const uint32_t raw_word) { | ||
return spvFixWord(raw_word, endianness); | ||
}); | ||
} else { | ||
words->insert(words->end(), _.words + _.word_index, | ||
_.words + index_after_operand); | ||
} | ||
words->insert(words->end(), _.native_words.get() + _.word_index, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment will need to change. This is not always translating to native endianness. |
||
_.native_words.get() + index_after_operand); | ||
} | ||
|
||
// Advance past the operand. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document what this means.