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

Add custom type vector support #1333

Open
wants to merge 32 commits into
base: develop
Choose a base branch
from
Open

Add custom type vector support #1333

wants to merge 32 commits into from

Conversation

geyyer
Copy link
Contributor

@geyyer geyyer commented Jun 12, 2024

  • add vectorization support for custom types defined as non-native types or user-defined objects
  • add a test to check if native and non-native vector types take same expected space in memory

@geyyer geyyer marked this pull request as ready for review June 14, 2024 15:14
@geyyer geyyer requested a review from poyenc as a code owner July 9, 2024 21:17
@illsilin illsilin requested a review from bartekxk as a code owner July 16, 2024 17:52
Copy link
Collaborator

@aosewski aosewski left a comment

Choose a reason for hiding this comment

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

Good work! I'd add few more test cases and maybe reconsider some aliases naming.

include/ck/utility/data_type.hpp Outdated Show resolved Hide resolved
include/ck/utility/data_type.hpp Outdated Show resolved Hide resolved
include/ck/utility/data_type.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd check also for non-native-vector data alignment if it's really next_pow_of_2. Moreover I'd add also a test case for data type which is not just a thin wrapper around standard types, ie consider creating Complex data type with real & img members.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Formally we have to create vectors taking next_pow_of_2 space in memory. In fact compiler optimizes these data structures and they take only space they actually need. We check memory footprint in the tests. Indeed, I'll try to add a complex type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 678c398

Copy link
Collaborator

Choose a reason for hiding this comment

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

@geyyer As for checking the alignment I rather was thinking about using alignof or alignment_of expressions to test it, since we have custom alignment. I understand that the compiler may do some optimizations to that. But my reasoning is to make sure that in device memory those data types will have the alignment as expected. Otherwise we may have observe unexpected lower memory bandwidth due to not fulfilled memory read/write alignment requirement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume it would be sufficient to make a test case where you check alignment of vector holding different size of elements (ie of sizes we define aliases for: 2,4,8,16,32,64,128,256) of just single data type.

const int size = 4;
std::vector<bool> test_vec = {false, true, false, true};
// reference vector
vector_type<custom_bool_t, size> right_vec;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also check here if all values were properly default initialized. They all should have value equal to the result of calling default constructor custom_bool_t() : data{type{}} {} . Similar for all other test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand your idea correctly, that's exactly what we check in these tests: we create a std vector with some values (test_vec); assign values to a vector type (right_vec) element-wise; create another vector (left_vec) using the default constructor; compare the new vector (left_vec) with the reference vector (test_vec). This way we can check both default constructor and AsType method. Please let me know if I missed something!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant that after this line vector_type<custom_bool_t, size> right_vec; all values of right_vec should have been default initialized - that is have a default value after calling default constructor - (usually zero for numeric types, boolean is false). Since we have vector of values, then I'd expect that all elements after this line have default value.

Copy link
Collaborator

@aosewski aosewski left a comment

Choose a reason for hiding this comment

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

One more note - you're checking AsType for just single vector element type. Please add test case where you call AsType for all vector type access combination/views.

@geyyer
Copy link
Contributor Author

geyyer commented Sep 17, 2024

Added vector reshaping tests in dc7208c

const int size = 4;
std::vector<bool> test_vec = {false, true, false, true};
// reference vector
vector_type<custom_bool_t, size> right_vec;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant that after this line vector_type<custom_bool_t, size> right_vec; all values of right_vec should have been default initialized - that is have a default value after calling default constructor - (usually zero for numeric types, boolean is false). Since we have vector of values, then I'd expect that all elements after this line have default value.

test/data_type/test_custom_type.cpp Outdated Show resolved Hide resolved

using type = d8_t;

union
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why you don't add here alignment of this union like in previous smaller vector types? alignas(next_pow2....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed in d08a8c5

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.

3 participants