Skip to content

Conversation

@aelovikov-intel
Copy link
Contributor

After #1770 some uses of associateWithHandler
have to be guarded by "#ifndef SYCL_DEVICE_ONLY" as the accessor's
inheritance chain differs between host/device compilation.

Put the burden of that into the handler's implementation instead of paying the
price at uses.

After intel#1770 some uses of associateWithHandler
have to be guarded by "#ifndef __SYCL_DEVICE_ONLY__" as the accessor's
inheritance chain differs between host/device compilation.

Put the burden of that into the handler's implementation instead of paying the
price at uses.
@aelovikov-intel aelovikov-intel requested a review from a team as a code owner July 15, 2022 23:28
@aelovikov-intel aelovikov-intel requested a review from againull July 15, 2022 23:28
againull
againull previously approved these changes Jul 15, 2022

class AccessorBaseHost;

#ifndef __SYCL_DEVICE_ONLY__
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor readability improvement: when there is an else branch, it's better to avoid negation. I.e. prefer

#ifdef
X
#else
Y
#endif

to

#ifndef
Y
#else
X
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually, I would have done that already. However, in cases when one branch is much more important/interesting than the other, the decision isn't so clear. Also, pre-existing usage of this in the file (beyond the affected lines) is such that the order is determined by what code it is (host - host is the first condition, device, inside parallel_for - device is the first condition), so I'm not sure if that's a positive change.

Anyway, I'm committed the suggested change. CodeOwners are free to either merge it or revert and merge the previous version.

@againull againull self-requested a review July 18, 2022 18:55
@againull againull merged commit 531e18b into intel:sycl Jul 18, 2022
@aelovikov-intel aelovikov-intel deleted the associateWithHandler branch July 28, 2022 20:31
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