Skip to content

Conversation

@kmpeng
Copy link
Contributor

@kmpeng kmpeng commented Nov 13, 2025

Closes #266.

Adds tests for ByteAddressBuffer's templated load/store member functions. These test one 32-bit type (uint), bool, one 16-bit type (int16_t), one 64-bit type (int64_t), a struct with an array in it, a small struct, and a big struct. I didn't test every single type of scalar since we're mostly trying to test the various different shapes that could be used.

@kmpeng kmpeng force-pushed the byteaddressbuffer-tests branch from c10505f to 801b497 Compare November 13, 2025 10:54
Copy link

@alsepkow alsepkow left a comment

Choose a reason for hiding this comment

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

Finn/Alex pair review: Left a few minor comments.

@kmpeng
Copy link
Contributor Author

kmpeng commented Nov 17, 2025

I have a question for people reviewing this: ByteAddressBuffer load/stores work with arrays in DXC (and explicitly tests them), but we agreed to move towards disallowing array returns in 202x. Should I keep the array test here? And should I implement array support for these methods in Clang?

@bogner
Copy link
Collaborator

bogner commented Nov 18, 2025

I have a question for people reviewing this: ByteAddressBuffer load/stores work with arrays in DXC (and explicitly tests them), but we agreed to move towards disallowing array returns in 202x. Should I keep the array test here? And should I implement array support for these methods in Clang?

We discussed this elsewhere but for posterity I'll note it here: no, we shouldn't test the array behaviour or implement the array behaviour in clang if we're disallowing array returns in 202x. Tests that we reject array versions should be written, but those belong in the clang/dxc repos and don't need to be covered in the offload test suite here.

@kmpeng kmpeng force-pushed the byteaddressbuffer-tests branch from 106fb3f to 6458811 Compare November 19, 2025 23:34
Copy link

@alsepkow alsepkow left a comment

Choose a reason for hiding this comment

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

LGTM

@kmpeng kmpeng merged commit 958565b into llvm:main Nov 20, 2025
10 of 12 checks passed
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.

Add tests for ByteAddressBuffer's templated load/store member functions

3 participants