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

Add support for generating linkable SPIR-V modules #2013

Closed
wants to merge 2 commits into from

Conversation

aqnuep
Copy link

@aqnuep aqnuep commented Dec 5, 2019

Added support for generating linkabe SPIR-V modules from shaders that only go through the compile stage without linking.

Due to the nature of GLSL this only enables function linkage, because global variable linkage in GLSL are so called "shared globals" which don't have a canonical copy, thus don't map well to SPIR-V's Linkage capability which works in a traditional export/import model. However, in practice this should not limit possible uses.

The generated SPIR-V files will use the Linkage SPIR-V capability without enabling any additional SPIR-V extensions, though an explicit SPIR-V extension enabling the usage of the Linkage capability in SPIR-V shader modules would probably be preferable.

The resulting linkable SPIR-V modules can be linked using the spirv-link tool once KhronosGroup/SPIRV-Tools#3081 gets merged.

New tests have been added to cover the functionality, but this change also has an effect on the output of existing test cases (due to the new EOpLinkerFunction nodes in the compiled but not linked ASTs). The reasons I didn't include the baseline expected outputs for the new and existing tests are the following:

  • I'm not sure about the workflow on whether the regeneration of the baseline results is something the PRs should do or something that the approver does
  • did not want to pollute the PR with a huge number of files
  • it seems that some of the existing baseline results are not up to date (lots of successful test cases contain the text output "Validation failed" which may be the result of lack of initialization of GlslangResult::validationResult)

The final piece of the puzzle will be a PR to shaderc which hopefully will solve issue google/shaderc#315.

@aqnuep
Copy link
Author

aqnuep commented Dec 5, 2019

  • it seems that some of the existing baseline results are not up to date (lots of successful test cases contain the text output "Validation failed" which may be the result of lack of initialization of GlslangResult::validationResult)

Indeed it looks like there's some uninitialized variable issue which caused the mismatches when I ran the tests without the change in this PR, as appveyor didn't generate the same failures due to the missing "Validation failed" message in the outputs, like my local build with MSVC.

I'll include the updated and new expected outputs in the PR with a second commit (again, to allow reviewing the PR without the data pollution).

I'll also try to figure out where the variable initialization is missing in the test suite.

@aqnuep
Copy link
Author

aqnuep commented Dec 6, 2019

Ok, now it passes all tests.

One thing to note is that I did not enable exporting the entry point functions themselves. If that would be desirable I can update the code.

@johnkslang
Copy link
Member

SPIR-V explicitly does not allow compilation unit linkage. For graphical stages it requires a fully linked stage within a single module. This is for a few reasons:

  • Different HLLs have different inter-unit linkage rules. SPIR-V needs a set of rules designed to support the popular HLLs we know are being used.
  • It introduces a new form of SPIR-V, while the standard is attempting to define a single SPIR-V for both distribution and acceptance by drivers.

Some pretty important extension or specification work needs to be done to resolve these.

I'm surprised SPIR-V Tools would be going against the specification as well; maybe I'm missing something.

@johnkslang
Copy link
Member

Also note that Vulkan excludes the Linkage capability from its list of allowed capabilities.

@aqnuep
Copy link
Author

aqnuep commented Dec 6, 2019

Okay, I need to clarify a few things (I've sent you an e-mail about this on Sunday, but I didn't get a response):

  • The generated (unlinked) SPIR-V modules are still specific to a particlar shader stage, and this solution is so far designed to work with glslang-generated content only (due to the custom name mangling and parameter passing convention of it). Sure, a well-defined ABI would be the ultimate solution, but within a single compiler stack things should work fine without it (just like all the C++ compiler stacks do).
  • These modules are not expected to be accepted by Vulkan drivers, but rather need subsequent linking either using spirv-link, or by using the to-be-implemented linking functionality in shaderc, which then become standard SPIR-V shader modules accepted by Vulkan.
  • Indeed it introduces a new form of SPIR-V (you can call it SPIR-V object file) which preferably need some specification work to become an official form, details TBD.
  • The spirv-link tool in SPIRV-Tools has been originally added to allow linking of kernels, but it can trivially work with graphics shaders as well, thus taking advantage of it here.

I suggest you to talk with @dneto0, as we had some preliminary conversation with him about this.

@RikoOphorst
Copy link

@aqnuep @johnkslang What is the status of this feature? Is there any ETA on when this might get merged to master?

@johnkslang
Copy link
Member

Waiting for a GLSL-level specification of what the linkage semantics are and how they mapped to SPIR-V.

Note that GLSL and C don't have the same semantics for how to combine what might appear to the be same object in different compilation units. The semantics are currently handled by AST merging. I think this PR subsets those, and to support the code, there should be specification for it.

@RikoOphorst
Copy link

@johnkslang Right, makes sense. Is there a link or reference you could give me to the internal Khronos threads about that on GitLab? I'm struggling to find the relevant info there.

@johnkslang
Copy link
Member

I think it can be posted here, looking to @aqnuep for that.

@aqnuep
Copy link
Author

aqnuep commented Feb 10, 2020

This is partially news to me too.
Originally I was requested to write a SPIR-V extension (not a GLSL one) for this and I received no response when asking for clarification in e-mail on what could possibly go into it (considering that the SPIR-V spec allows this as is) and that was a while ago. So I wasn't sure how to proceed with this.

Sure, I guess writing a GLSL extension for documenting the mapping from GLSL to SPIR-V (without introducing any new capabilities) makes sense. I assume it would practically act as a documentation of the glslang implementation of the mapping (sort of like a partial ABI).

That sound like a reasonable plan!
However, it's been a while and I mostly gave up on getting approval of this (it was a hobby project anyway) and I'm not sure when I would have sufficient time to progress it any further, so @RikoOphorst, in case you would like to have further progress on this you may have to take matters into your hand (sorry).

P.S.: I may also have most of the changes needed on the shaderc side of things lying around somewhere, so I can share that too if needed.

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@arcady-lunarg
Copy link
Contributor

@aqnuep do you still plan to work on this? If not, we would like to close the PR.

@aqnuep
Copy link
Author

aqnuep commented May 18, 2023

No, @arcady-lunarg. This one is really old at this point and there seems to be no traction to make this happen. Closing.

@aqnuep aqnuep closed this May 18, 2023
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

5 participants