Skip to content
This repository was archived by the owner on Oct 31, 2025. It is now read-only.

added memory barrier intrinsics to spirv-std#769

Merged
khyperia merged 6 commits intoEmbarkStudios:mainfrom
hannes-vernooij:memory-barrier-intrinsics
Oct 26, 2021
Merged

added memory barrier intrinsics to spirv-std#769
khyperia merged 6 commits intoEmbarkStudios:mainfrom
hannes-vernooij:memory-barrier-intrinsics

Conversation

@hannes-vernooij
Copy link
Copy Markdown
Contributor

@hannes-vernooij hannes-vernooij commented Oct 18, 2021 •
edited
Loading

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.

repi
repi suggested changes Oct 18, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

khyperia commented Oct 18, 2021

Additionally, please add some compiletests!

@hrydgard
Copy link
Copy Markdown
Contributor

hrydgard commented Oct 18, 2021

Linking to related issue #696

@hrydgard hrydgard linked an issue Oct 18, 2021 that may be closed by this pull request
Memory barrier documentation, and wrappers for common usage #696
Closed
@hannes-vernooij
added function-level docs
f7fd8d0
@hannes-vernooij
added unit-tests
55614c4 
constants could not be validated in tests because of the limited output
@hannes-vernooij
Copy link
Copy Markdown
Contributor Author

hannes-vernooij commented Oct 19, 2021

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?

khyperia
khyperia approved these changes Oct 20, 2021
View reviewed changes
Copy link
Copy Markdown
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
hrydgard
hrydgard approved these changes Oct 20, 2021
View reviewed changes
Copy link
Copy Markdown
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.

repi
repi suggested changes Oct 20, 2021
View reviewed changes
@hannes-vernooij
Copy link
Copy Markdown
Contributor Author

hannes-vernooij commented Oct 21, 2021

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
Copy Markdown
Contributor

khyperia commented Oct 21, 2021

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
Copy Markdown
Contributor Author

hannes-vernooij commented Oct 21, 2021

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
Copy Markdown
Contributor

khyperia commented Oct 21, 2021 •
edited
Loading

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eddyb eddyb Awaiting requested review from eddyb

3 more reviewers

@hrydgard hrydgard hrydgard approved these changes

@khyperia khyperia khyperia approved these changes

@repi repi repi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory barrier documentation, and wrappers for common usage

4 participants

@hannes-vernooij @khyperia @hrydgard @repi