Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Hugobros3
Copy link

@Hugobros3 Hugobros3 commented Jul 1, 2023

The SPIR-V specifications is agnostic on the issue of endianness (at least at the 32-bit word level), and tooling has some supporting code for it. It sadly appears to suffer from quite a bit of rot, this PR tackles the following:

  • spirv-dis fails to disassemble a SPIR-V file in the non-native endianness
    • This is caused by trying to parse a string literal before the words are converted into native order, as assumed by MakeString
    • I changed it so that the whole file is byte-swapped when the parser is initialized, which opens the path for further simplifications
  • source extraction code duplicates the logic added in mhillenbrand@a95b262 and fails to account for mismatched endianness
  • Some amount of dead code resulting from the prior commit

I ran and passed all tests on a real PowerPC G5, which is as fun a way to spend a day as any other. There's probably more I can do, but I'd like this to be reviewed before just so I know I'm on the right track.

Additionally, I'm not sure how relevant big-endian SPIR-V support really is considering the state of the tooling. Rather it would probably be beneficial to have big-endian architectures default to emitting little-endian files, as I doubt there are many, if any implementations that correctly implement endianness agnosticism. I think it needs to at least be an option!

@CLAassistant
Copy link

CLAassistant commented Jul 1, 2023

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants