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

Update to upstream SPIR-V 1.5.2 #132

Merged
merged 7 commits into from Jun 21, 2020

Conversation

Jasper-Bekkers
Copy link
Collaborator

@Jasper-Bekkers Jasper-Bekkers commented Jun 18, 2020

This removes the internal version of spirv.core.grammar.json and switches to use only the upstream one spirv.core.grammar.json file as discussed in #116 as a potential better way forward.

This also updates all the bindings to SPIR-V 1.5.2

@Jasper-Bekkers
Copy link
Collaborator Author

Looks like I can't add reviews but @antiagainst and @kvark should look at this.

@kvark
Copy link
Member

kvark commented Jun 18, 2020

Thank you, that looks like an outstanding piece of work!

Remove extra dbg! statements.
@Jasper-Bekkers
Copy link
Collaborator Author

Looks like rspirv/dr/build/autogen_norm_insts.rs isn't generated properly.

@Jasper-Bekkers
Copy link
Collaborator Author

I think there may be a bit more work to do - it's a pretty big change in a mostly unfamiliar codebase so an extra pair of eyes would definitely help, but I think I have a significant part of it covered.

Do we have some kind of automated test suite to test against?

@kvark
Copy link
Member

kvark commented Jun 19, 2020

@Jasper-Bekkers do you exist on the Matrix for a short chat, e.g. on #gfx:matrix.org?
The situation is a bit troublesome:

  1. the project has 3 levels of complexity on top of each other: bindings, data representation (DR), structured representation (SR)
  2. @antiagainst - the creator of the project - has little time to develop this, but they can look at changes sometimes
  3. we (gfx-rs org) wanted to took over the project and use it for our shader translation needs, contributing to the SR heavily. However, we gave up and are no longer interested in either DR or SR in rspirv. We only use spirv-headers from rspirv. We are building the swizz army knife of shader translation in https://github.com/gfx-rs/naga now.

Therefore, there is hardly anyone to help you land these changes and review them (with respect to DR/SR parts), unfortunately (sorry!).
There is a solution, however. If @antiagainst doesn't mind, I can add you as a maintainer, and you'll be able to shape the DR/SR more towards your needs. Perhaps, it would make sense to move the whole project under Embark, even?
Let me know what you think.

@kvark
Copy link
Member

kvark commented Jun 19, 2020

@antiagainst how do you feel about spirv-headers being moved out to the Khronos github organization?
It looks like DR/SR will be left unmaintained after that, but it's better than trying to drag them along at this point.

@Jasper-Bekkers
Copy link
Collaborator Author

Thanks @kvark for making me a maintainer of this project. I too mostly use it for spirv-headers and have done the last two updates now to new versions of SPIR-V so I think it makes a lot of sense.

I've been developing a bit against this version of the library and I haven't run into any oddities at least. Would be good to get some eyes on this and #133 and then merge them in and publish a new version.

@Jasper-Bekkers
Copy link
Collaborator Author

Since I'm starting to get PRs to my fork of this work, I think I'll go ahead and merge this to mainline to prevent fork-of-fork situations. If anyone objects, feel free to reach out to me or revert the PR.

@Jasper-Bekkers Jasper-Bekkers merged commit 220c9b4 into gfx-rs:master Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants