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

[spirv] Add Vk::RawBufferLoad intrinsic #4069

Merged

Conversation

yuriy-odonnell-epic
Copy link
Contributor

@yuriy-odonnell-epic yuriy-odonnell-epic commented Nov 8, 2021

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 HLSL ByteAddressBuffer (4 byte alignment).

Example shader:

uint64_t Address;
float4 main() : SV_Target0 {
  uint Value = vk::RawBufferLoad(Address);
  return asfloat(Value);
}

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.

@yuriy-odonnell-epic
Copy link
Contributor Author

Potentially, custom alignment tagging on load ops could be implemented using SPIR-V intrinsics that are being added in #3919.

@jaebaek jaebaek self-requested a review November 8, 2021 21:21
@jaebaek jaebaek added the spirv Work related to SPIR-V label Nov 8, 2021
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@yuriy-odonnell-epic yuriy-odonnell-epic force-pushed the spirv_raw_buffer_reference branch 2 times, most recently from d9e0a0a to 442ce3f Compare November 9, 2021 14:57
@AppVeyorBot
Copy link

@yuriy-odonnell-epic
Copy link
Contributor Author

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.

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@yuriy-odonnell-epic yuriy-odonnell-epic changed the title [spirv] [WIP] Add Vk::RawBufferLoad intrinsic [spirv] Add Vk::RawBufferLoad intrinsic Nov 10, 2021
@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

Copy link
Collaborator

@jaebaek jaebaek left a 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!

include/dxc/HlslIntrinsicOp.h Show resolved Hide resolved
SpirvInstruction *address = doExpr(addressExpr);

const HybridPointerType *bufferType =
spvBuilder.getPhysicalStorageBufferType(astContext.UnsignedIntTy);
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jaebaek
Copy link
Collaborator

jaebaek commented Nov 16, 2021

By the way, I observed that running clang-format slightly changes the code format.
Could you please apply this patch?
diff.txt

I got it by running

git clang-format master
git diff > diff.txt

* 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
@yuriy-odonnell-epic
Copy link
Contributor Author

Applied the getUIntType and clang format changes.

@AppVeyorBot
Copy link

Copy link
Collaborator

@jaebaek jaebaek left a 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!

@jaebaek jaebaek merged commit f836555 into microsoft:master Nov 17, 2021
@yuriy-odonnell-epic
Copy link
Contributor Author

Thanks for reviewing and merging! I will work on the vectorized, templated and aligned loads next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spirv Work related to SPIR-V
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants