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

[tests] Add Input Data Sweep tests #1429

Merged
merged 30 commits into from
Apr 25, 2024
Merged

Conversation

danhoeflinger
Copy link
Contributor

@danhoeflinger danhoeflinger commented Mar 1, 2024

Note: Should be merged after #1445 and #1438(merged), as errors can occur otherwise.

Summary:
Adding test coverage to catch bugs which slipped through our previous testing by targeting a sweep of interesting combinations of input data. Would have detected the following bugs which were missed by our test coverage at the time:

#1413 (f69618b) reverse_iterator: detects as a runtime failure.
#1371 (286097b) zip_iterator device copyable: detects as a build error
#1383 (a860b5d) 3 fixes for permutation iterator: detects as build error
#1341 (fef0958) "general case" with recursion of permutation iterator: detects as build error
#1325 (6d50dba) broken bracket operator permutation iterator: detects as build error


Details:
The test does not specifically target these bugs listed above, but rather identifies the target base data, wrappers, and recurses to generate a large number of possible iterator types. It runs these resulting iterator types through a minimal kernel (copy of 10 elements) as both a read and write and checks the result. Some types and wrappers disable tests or aspects of the resulting tests for deeper levels of recursion.

Base Types:

  1. float (recurse 0)
  2. double (recurse 0)
  3. uint64_t (recurse 0)
  4. int32_t (recurse 2 layers of wrappers)

Sequence types:

  1. host_iterator
  2. sycl_iterator
  3. counting_iterator (for integral types)
  4. usm_shared
  5. usm_device
  6. std::vector<T,usm_allocator<T,sycl::usm::alloc::shared>>::iterator
  7. std::vector<T,usm_allocator<T,sycl::usm::alloc::host>>::iterator

Wrappers:

  1. std::reverse_iterator(it)
  2. transform_iterator(it, noop{})
  3. permutation_iteartor(it,noop{})
  4. permutation_iterator(it, counting_iter)
  5. permutation_iterator(counting_iterator, it)
  6. zip_iterator(counting_iterator, it)
  7. zip_iterator(it, discard_iterator)

It also explores a couple special cases each sent through 1 level of recursion:

  1. discard_iterators
  2. permutation_iterator( permutation_iterator( usm_shared<int>, counting_iterator), counting_iterator)

This PR catches 2 issues

  1. build error for zip_iterator<it, transform_iterator<it,lambda> inputs, which is fixed by transform_iterator improvements, constructors, copy assignment, docs, tests #1445.

  2. Inefficient and potentially incorrect handling of std::vector<T,usm_allocator>::iterator. This has been addressed by Adding std::vector<T, usm_allocator>::iterator to is_passed_directly types #1438.
    Note: Depending on implementation of std::vector, some std::vector<T>::iterator cannot be distinguished from std::vector<T, usm_allocator>::iterator. For those cases, we must continue handling these types as if they were host_iterators.

Note:
I've disabled some combinations which don't make sense including std::reverse_iterator(sycl_iterator based "iterators") because sycl_iterators are not iterators. In practice, those do seem to work on linux, but not windows. There are build errors on windows when attempting to run the SFINAE trick for __is_addressable.

@danhoeflinger danhoeflinger force-pushed the dev/dhoeflin/input_data_sweep branch 4 times, most recently from b6e5387 to 31da5f2 Compare March 4, 2024 16:36
@danhoeflinger danhoeflinger marked this pull request as ready for review March 4, 2024 17:23
@danhoeflinger danhoeflinger marked this pull request as draft March 4, 2024 18:33
@danhoeflinger danhoeflinger marked this pull request as ready for review March 5, 2024 15:20

#if TEST_DPCPP_BACKEND_PRESENT
template <int __recurse, int __reverses, bool __read = true, bool __reset_read = true, bool __write = true,
bool __check_write = true, bool __usable_as_perm_map = true, bool __usable_as_perm_src = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Practically what's mean when we have template params with some default values before (!) some other template params without default values? Are we able to pack all these template params with default values into some type trait or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe you are allowed to treat template types and values independently, and can have defaults like this without issue.
Yes, it would perhaps be better to collapse and pack these into a single value. It has grown quite large, I'll see what I can come up with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a good suggestion for cleaning this up without over-engineering or obfuscating the tests?

It needs to be compile time, the configuration is cumulative (more an more parts of the test may get turned of with layers of recursion), and its not necessarily easy to pull apart the final wrapped iterators to get to their fundamental sequence / important wrappers to determine these values from the types themselves.

I could group them into a single struct, but that doesn't change much. Its ugly as is, but I don't think its confusing especially with the comments. I'm inclined to leave it as is unless there is a more specific suggestion of how to improve it.

@danhoeflinger
Copy link
Contributor Author

I've split this into individual tests for each input sequence type (host_iter, sycl_iterator, usm_device, usm_shared, counting_iter). When grouped together, there is some potential for running out of memory during compilation in some environments.

Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
…ed as the source for permutation iterators

Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger danhoeflinger merged commit 1f3d4e7 into main Apr 25, 2024
20 checks passed
@danhoeflinger danhoeflinger deleted the dev/dhoeflin/input_data_sweep branch April 25, 2024 16:46
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.

Improve coverage of input data processing types in oneDPL unit testing with dpcpp backend
3 participants