Skip to content

[SYCL] Force instantiation of parallel_for before use of KernelInfo #3886

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 6 commits into from
Closed

[SYCL] Force instantiation of parallel_for before use of KernelInfo #3886

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7fae328
[SYCL] Force instantiation of parallel_for before use of KernelInfo
Jun 4, 2021
6c94611
Forgot to mark parallel_for static
Jun 4, 2021
a401090
Extract the instantiator out into its own type to simplify its use
Jun 4, 2021
9bb9bcd
Clang-format fix
Jun 4, 2021
f60aaa7
Revert most of the change, just remove the
Jun 4, 2021
70d05d1
Add a few regression tests
AlexeySachkov Jun 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions sycl/include/CL/sycl/handler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,19 +796,11 @@ class __SYCL_EXPORT handler {

// Get the kernal name to check condition 3.
std::string KName = typeid(NameT *).name();
using KI = detail::KernelInfo<KernelName>;
bool DisableRounding =
(getenv("SYCL_DISABLE_PARALLEL_FOR_RANGE_ROUNDING") != nullptr) ||
(KName.find("SYCL_DISABLE_PARALLEL_FOR_RANGE_ROUNDING") !=
std::string::npos) ||
(KI::getName() == nullptr || KI::getName()[0] == '\0') ||
(KI::callsThisItem());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned about this particular change. Not calling this_item free functions is a requirement for range rounding feature. Having all tests passed means that we don't have coverage for this.

@romanovvlad, @rdeodhar, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

The test SYCL/Basic/free_function_queries/free_function_queries.cpp tested this. However, for performance reasons, the heuristics for range rounding were later adjusted so that rounding is only done when the range >= 1024. The test uses a range of 10 so the kernel that uses this_item is not subject to range rounding and the test passes.
The test will need to be modified to increase the range (I will do it separately).
So removing that check from handler.hpp is not correct.


// Perform range rounding if rounding-up is enabled
// and there are sufficient work-items to need rounding
// and the user-specified range is not a multiple of a "good" value.
if (!DisableRounding && (NumWorkItems[0] >= MinRangeX) &&
(NumWorkItems[0] % MinFactorX != 0)) {
if ((NumWorkItems[0] >= MinRangeX) && (NumWorkItems[0] % MinFactorX != 0)) {
// It is sufficient to round up just the first dimension.
// Multiplying the rounded-up value of the first dimension
// by the values of the remaining dimensions (if any)
Expand Down