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

[oneDPL] Declare predefined dpcpp_default policy as const #554

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented Jul 9, 2024

(Edited by @akukanov)
This patch fixes what we consider to be a bug in the oneDPL specification, namely - the predefined dpcpp_default execution policy not being const.

Since all explicitly specified member functions of the device_policy class are const, a mutable policy object differs from a non-mutable (const) policy object in two ways:

  • when an lvalue, it can be a target to assign (i.e. copy or move) another policy object to, changing the associated SYCL queue to the queue associated with the other policy;
  • when an rvalue, it can be moved to another policy object, which may invalidate the associated queue and leave the moved-from policy in an unspecified state.

Neither of the above is an appropriate modification for dpcpp_default, potentially changing its semantics as a predefined policy associated with the default SYCL device. The possibility of such modifications was previously overlooked and deserves to be fixed.

The change will not impact the ability to pass dpcpp_default to oneDPL algorithms, to obtain its associated queue, and to copy or rebind the policy (including using it as the argument to the make_device_policy function), which constitute the vast majority of its usages in application codes. Codes that might be broken by the change either do not properly handle any const instances of the class or depend on the possibility to modify dpcpp_default, which we do not consider as a proper use and want to prevent. In the latter case, a workaround can be to create a copy of dpcpp_default and use that instead.

@SergeyKopienko
Copy link
Contributor Author

@akukanov Why fpga_policy is not described in our specification?

@akukanov akukanov changed the title Declare predefined dpcpp_default and dpcpp fpga policies as const Declare predefined dpcpp_default policy as const Jul 9, 2024
@akukanov
Copy link
Contributor

akukanov commented Jul 9, 2024

Why fpga_policy is not described in our specification?

There is neither enough implementation experience nor enough demand to justify adding special FPGA policies to the oneDPL specification.

…the way dpcpp_default is constructed should not be prescribed in the specification.
@danhoeflinger
Copy link
Contributor

Lets mark the title of this PR with [oneDPL] to match the convention established so far.

@SergeyKopienko SergeyKopienko changed the title Declare predefined dpcpp_default policy as const [oneDPL] Declare predefined dpcpp_default policy as const Jul 10, 2024
@SergeyKopienko
Copy link
Contributor Author

Lets mark the title of this PR with [oneDPL] to match the convention established so far.

Done.

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM

@akukanov akukanov merged commit 147c900 into uxlfoundation:main Aug 6, 2024
3 checks passed
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