Prevent signed integer overflow in check_scan_over_group
#776
Conversation
Check that the maximum value of `T` is large enough to store the results of the scan functions. This check prevents overflow for small types. Signed-off-by: Michael Aziz <[email protected]>
tests/group_functions/group_scan.h
Outdated
| if (std::sqrt(std::numeric_limits<T>::max()) + T(init) <= range.size()) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think it's a good idea silently skipping all checks in this case.
Although this sounds like a test issue, rather than a bug in the tested implementation, we probably can use REQUIRE macro to "Check that the maximum value of T is large enough to store the results of the scan functions.".
There was a problem hiding this comment.
Could you please clarify how you’re suggesting we use the REQUIRE macro? I understood that this macro would cause the test to fail if the assertion is false.
The change I’m proposing should only skip cases where the type is not large enough for the specified range. The type will still be tested with the smaller ranges.
There was a problem hiding this comment.
Why do we launch the configuration causing the overflow? Isn't this a bug?
There was a problem hiding this comment.
I think that the relevant code is here:
SYCL-CTS/tests/group_functions/group_scan.h
Line 241 in 47a0517
We use the same template for all of the test cases so I’m not able to remove the problematic cases without changing the other working tests.
There was a problem hiding this comment.
Right. This where I think we should change the logic to avoid an overflow.
work_goup_size here is the maximum possible # of work-items the device can execute. Do we really need to the maximum # of work-items to test the scan functionality? If so, the input buffer must be initialized with zeros to avoid overflow.
But I don't think we need to run so many work-items, it's a good stress test, but an overkill for this test purpose.
There was a problem hiding this comment.
Right. This where I think we should change the logic to avoid an overflow.
work_goup_size here is the maximum possible # of work-items the device can execute. Do we really need to the maximum # of work-items to test the scan functionality?
I believe that the larger test case is not needed. I've removed it and updated the PR description.
If so, the input buffer must be initialized with zeros to avoid overflow.
I don't think I understand this suggestion. The test case is computing the sum of an arithmetic progression, but the result may be too large to store in certain types. I don't think that the issue is related to buffer initialization.
BTW, it's better to use the sum of arithmetic progression instead of sqrt for the overflow check.
I've updated the overflow check to use this formula.
Signed-off-by: Michael Aziz <[email protected]>
Signed-off-by: Michael Aziz <[email protected]>
tests/group_functions/group_scan.h
Outdated
| @@ -238,7 +238,7 @@ struct init_joint_scan_group { | |||
|
|
|||
| size_t work_group_size = work_group_range.size(); | |||
|
|
|||
| const size_t sizes[3] = {5, work_group_size / 2, 3 * work_group_size}; | |||
| const size_t sizes[3] = {5, work_group_size / 2}; | |||
There was a problem hiding this comment.
Either declare 2-element array or explicitly initialize the 3rd element.
Why the second element is work_group_size / 2? Why can't we put 1 or 2 instead (+ check that HW is able to run the work of that size)?
There was a problem hiding this comment.
Either declare 2-element array or explicitly initialize the 3rd element.
Good catch! I've updated the array declaration to include the correct number of elements.
Why the second element is
work_group_size / 2? Why can't we put 1 or 2 instead (+ check that HW is able to run the work of that size)?
I'm not sure why the second size was work_group_size / 2, but I've set it to 2 instead.
There was a problem hiding this comment.
That means which should even more comment the code.
Signed-off-by: Michael Aziz <[email protected]>
|
Ivan, please have a look. Thx. |
Signed-off-by: Michael Aziz <[email protected]>
|
I'll wait for the feedback from @Melirius to merge. |
|
Will wait one more week. |
|
Friendly reminder for @Melirius. |
Sorry, I'm at vacations now, be back on the next week. |
@Melirius, friendly reminder №3. We have required approvals, so it's ready to be merged. I'm going to merge this PR next Wednesday if there are no additional requests for changes. |
Check that the maximum value of
Tis large enough to store the results of the scan functions. This check prevents overflow for small types. Also, remove the largestcheck_scantest case.