Reduce the test scope for sort.cpp #670
Conversation
This test takes ~50 sec to run in CI system. There should be no value in running multiple problem sizes to validate that sort works.
|
@capatober, FYI. |
|
@bader, thanks for informing me about these changes! Regarding the patch. The thing is that group sorting algorithm has different behavior depending on size of sequence (as other group algorithms):
To reduce testing let's keep just 3 values: 1, 256, 1024. These 3 cases are matched to aforementioned 3 points. Other values are duplication. Let's see how else we can reduce the testing.
|
|
@capatober, I've reduced work-group size and applied minor style fixes. Please, take a look. |
| @@ -38,14 +36,11 @@ struct CustomFunctor { | |||
| } | |||
| }; | |||
|
|
|||
| // we need it since using std::abs leads to compilation error | |||
| template <typename T> T my_abs(T x) { return x >= 0 ? x : -x; } | |||
There was a problem hiding this comment.
I'm ok with the change if there are no compilation errors related to abs as it's mentioned in the comment
There was a problem hiding this comment.
As you can see in pre-commit logs, there no compilation errors. BTW, the comment is about std::abs, not sycl::abs.
I trusted your comment and didn't try to use std::abs here.
There was a problem hiding this comment.
I see. Sorry, missed that there is sycl::abs now.
There was a problem hiding this comment.
I checked std::abs and I see the problem.
llvm-test-suite/SYCL/GroupAlgorithm/SYCL2020/sort.cpp:40:10: error: call to 'abs' is ambiguous
return std::abs(lhs - rhs) > epsilon;
^~~~~~~~
/llvm-test-suite/SYCL/GroupAlgorithm/SYCL2020/sort.cpp:49:9: note: in instantiation of function template specialization 'check<unsigned int>' requested here
if (check(expected[i], got[i], epsilon)) {
^Standard library doesn't provide abs overloads for unsigned integer types. It make sense, so the question is: why SYCL spec defines abs for unsigned integer types?
I suppose your code might not work as expected for unsigned types. I suggest you to think about it.
There was a problem hiding this comment.
Thanks for pointing it out. What is your opinion whether sycl::abs for unsigned integer types is a bug or a feature of SYCL?
There was a problem hiding this comment.
Technically, I don't any problems with defining such function, but the use case is unclear. This function does nothing, but return its only argument.
I suppose it wasn't intention to define such function. It's just the side effect of listing all supported OpenCL types as allowed parameters for integer functions.
@intel/dpcpp-specification-reviewers, FYI.
There was a problem hiding this comment.
I captured this question as a Kronos issue against the SYCL spec: https://gitlab.khronos.org/sycl/Specification/-/issues/584
| @@ -38,14 +36,11 @@ struct CustomFunctor { | |||
| } | |||
| }; | |||
|
|
|||
| // we need it since using std::abs leads to compilation error | |||
| template <typename T> T my_abs(T x) { return x >= 0 ? x : -x; } | |||
There was a problem hiding this comment.
I see. Sorry, missed that there is sycl::abs now.
This test takes ~50 sec to run in CI system, whereas most of other tests executed in < 5 sec. There should be no value in running multiple problem sizes to validate that sort works.
This test takes ~50 sec to run in CI system, whereas most of other tests executed in < 5 sec.
There should be no value in running multiple problem sizes to validate
that sort works.