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

added memory barrier intrinsics to spirv-std #769

Merged

Conversation

hannes-vernooij
Copy link
Contributor

@hannes-vernooij hannes-vernooij commented Oct 18, 2021

Added memory barrier intrinsics to get feature parity with GLSL and HLSL. Currently adding barriers is quite verbose and most used usecases are covered with these intrinsics. I used the DXC to compile the hlsl equivalents to SPIR-V.

Copy link
Contributor

@repi repi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Could you add some function-level docs for each of the barrier functions? Could describe it on a high-level and then maybe reference the GLSL/HLSL functions (as you do internally in the function body, but in the docs) and link to more details about them?

@hannes-vernooij hannes-vernooij force-pushed the memory-barrier-intrinsics branch from 44aa3c7 to 6101569 Compare October 18, 2021 15:39
@khyperia
Copy link
Contributor

Additionally, please add some compiletests!

@hrydgard
Copy link
Contributor

Linking to related issue #696

@hrydgard hrydgard linked an issue Oct 18, 2021 that may be closed by this pull request
constants could not be validated in tests because of the limited output
@hannes-vernooij
Copy link
Contributor Author

Additionally, please add some compiletests!

I added compile tests for all intrinsics but I was not able to validate the constants, they are probably defined outside of the scope.

%1 = OpFunction  %2  None %3
%4 = OpLabel
OpLine %5 40 4
OpControlBarrier %6 %7 %8
OpLine %9 8 1
OpReturn
OpFunctionEnd

As you can see in the example above %6, %7 and %8 are not output. Do you maybe have an idea on how I can get this incuded in scope for the tests?

Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, should have been more clear - I mostly wanted compiletests to make sure it just compiles/validates, rather than disassembling - just a compiletest (without --disassemble-xyz) is fine. How it is now is perfectly OK too, though!

Review-request @hrydgard to make sure the names here are OK, and no more are needed to solve your use cases.

@khyperia khyperia requested review from hrydgard and repi October 20, 2021 08:07
Copy link
Contributor

@hrydgard hrydgard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

crates/spirv-std/src/arch/barrier.rs Outdated Show resolved Hide resolved
@hannes-vernooij
Copy link
Contributor Author

There is one problem regarding the tests; the workgroup and all_memory barrier intrinsics currently rely on the VulkanMemoryModelDeviceScopeKHR and SPV_KHR_vulkan_memory_model features. I added these to the target_features defined in tests/src/main.rs but this causes some tests to fail due to the added OpCapabilities, Is it okay to re-generate and push these changes?

@khyperia
Copy link
Contributor

Hmm. You should not need to add the capabilities to tests/src/main.rs, but rather adding them to compile-flags: -C target-feature like other tests do. The outputs of other tests should not have to be updated.

@hannes-vernooij hannes-vernooij force-pushed the memory-barrier-intrinsics branch from 72c0522 to 2699d0a Compare October 21, 2021 12:27
@hannes-vernooij
Copy link
Contributor Author

Hmm. You should not need to add the capabilities to tests/src/main.rs, but rather adding them to compile-flags: -C target-feature like other tests do. The outputs of other tests should not have to be updated.

Oh sorry about that, I could have figured that out myself. I was thrown off a bit by the features above that were only used by read_cloack_khr.rs.

@khyperia
Copy link
Contributor

khyperia commented Oct 21, 2021

Yeah, in that case there were complications, which, should probably also happen here, but I'm way too tired :c and how it is now is totally fine

@khyperia khyperia merged commit 4831789 into EmbarkStudios:main Oct 26, 2021
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.

Memory barrier documentation, and wrappers for common usage
4 participants