-
Notifications
You must be signed in to change notification settings - Fork 722
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
[spirv] Add Vk::RawBufferLoad intrinsic #4069
[spirv] Add Vk::RawBufferLoad intrinsic #4069
Conversation
Potentially, custom alignment tagging on load ops could be implemented using SPIR-V intrinsics that are being added in #3919. |
4151a3a
to
0f144e5
Compare
d9e0a0a
to
442ce3f
Compare
The functionality, docs and test are now implemented. I am not planning to make any significant further changes, so now would be a good time to review. |
442ce3f
to
19b0c21
Compare
19b0c21
to
8e9253e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just a nit.
By the way, this PR clearly and precisely addresses a non-trivial issue, which is pretty impressive. I appreciate your contribution!
SpirvInstruction *address = doExpr(addressExpr); | ||
|
||
const HybridPointerType *bufferType = | ||
spvBuilder.getPhysicalStorageBufferType(astContext.UnsignedIntTy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of a curiosity, is there any reason that you do not use
const SpirvPointerType *bufferType =
spvBuilder.getPhysicalStorageBufferType(spvContext.getUIntType(32));
?
It is a minor issue, but we prefer SpirvPointerType
because const SpirvPointerType *SpirvContext::getPointerType(..)
internally keeps the pointers we already created and reuses it while const HybridPointerType *SpirvContext::getPointerType(..)
does not.
If there is no special reason, could you please replace it with SpirvPointerType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
By the way, I observed that running clang-format slightly changes the code format. I got it by running
|
* Only support raw buffer (i.e. HLSL ByteAddressBuffer) style loads of single uint-s * Always use 4 byte alignment for loads (same as HLSL ByteAddressBuffer) * Add PhysicalStorageBufferAddresses capability and KHR_physical_storage_buffer extension when needed * Promote memory addressing model to PhysicalStorageBuffer64 when needed * Add custom alignment support to SpirvLoad * Add createUnaryOp() overload that takes SpirvType * Add getPhysicalStorageBufferType() * Add documentation and a test case for the new intrinsic
8e9253e
to
c2ef55d
Compare
Applied the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for your contribution!
This PR clearly addresses the buffer address support!
Thanks for reviewing and merging! I will work on the vectorized, templated and aligned loads next. |
This PR adds a basic implementation of memory loads through
buffer_reference
mechanism in SPIR-V. Partially addresses #3772.Current implementation only handles basic
uint
loads, with similar semantics as HLSLByteAddressBuffer
(4 byte alignment).Example shader:
Proposed change implements the minimal functionality required for Unreal Engine and I would like to keep the scope of this particular PR minimal. Custom alignment, large/vector loads, templated load syntax, stores, etc. would also be great to implement (and I could do the work, if needed), but I would prefer to do them separately.
Update: The PR is now rebased and squashed, ready for review.