Skip to content
This repository was archived by the owner on Mar 28, 2023. It is now read-only.

[SYCL] Specified ONEAPI aliases in functions and function objects #300

Merged
merged 9 commits into from
Jun 4, 2021

Conversation

maximdimakov
Copy link

@maximdimakov maximdimakov commented May 28, 2021

Specified ONEAPI:: aliases in functions and function objects from ONEAPI namespace.
Providing the aliases for function objects is needed for intel/llvm#3868

@maximdimakov maximdimakov requested review from AlexeySachkov, Pennycook and a team as code owners May 28, 2021 08:26
@maximdimakov maximdimakov requested a review from vladimirlaz May 28, 2021 08:26
@maximdimakov
Copy link
Author

Not all the places with functions are here. If the changes will be allowed I'll add another

vladimirlaz
vladimirlaz previously approved these changes May 28, 2021
@vladimirlaz
Copy link

@maximdimakov could you please add motivation for the change? Is there any problem resolved?

@maximdimakov
Copy link
Author

@maximdimakov could you please add motivation for the change? Is there any problem resolved?

I changed the description of PR

@vladimirlaz
Copy link

@maximdimakov could you please add motivation for the change? Is there any problem resolved?

I changed the description of PR

As soon as sub_group interface has been moved to sycl:: isn't it reasonable to update tests accordingly
@Pennycook could you please comment/approve?

@maximdimakov
Copy link
Author

@vladimirlaz sub_group class will be moved to sycl:: but I will add aliases in ONEAPI:: so ONEAPI::sub_group will remain available.

@Pennycook
Copy link

@vladimirlaz sub_group class will be moved to sycl:: but I will add aliases in ONEAPI:: so ONEAPI::sub_group will remain available.

Are you saying that ONEAPI::sub_group will just be an alias to sycl::sub_group?

If so, I don't think that's a good idea. If I understand correctly, that would allow a developer to write something like this:

sycl::sub_group sg = it.get_sub_group(); // create a standard SYCL 2020 sub-group
auto x = sg.load(ptr); // use a member function from sycl::ONEAPI::sub_group

Would it prevent overloading a function on both sycl::ONEAPI::sub_group and sycl::sub_group? A user might want to do that.

The extension mechanism was designed to make it obvious when a user calls something that is not part of the standard -- in this case, they should create a different sub_group object in a different namespace -- and sidestepping that here seems dangerous. It also doesn't seem future-proof: we might need to change the interface for sycl::ONEAPI::sub_group or sycl::sub_group (or both), and having them reference the same class will make that more difficult.

I think my preferred solution would be to have sycl::sub_group and sycl::ONEAPI::sub_group exist as separate classes. That would still permit code re-use by moving the implementation of the classes somewhere else, and we could choose to make things easier on our users by allowing implicit conversions between the two classes. But by making them distinct classes we'd be honoring the spirit of the extension mechanism, and I think we'd avoid ADL traps like this.

Tagging @gmlueck for his opinion, since this pertains to the extension mechanism.

@gmlueck
Copy link

gmlueck commented Jun 1, 2021

If the ONEAPI version of sub_group has any differences from the sycl version, then we certainly don't want to create an alias because of the problem that @Pennycook notes:

sycl::sub_group sg = it.get_sub_group(); // create a standard SYCL 2020 sub-group
auto x = sg.load(ptr); // use a member function from sycl::ONEAPI::sub_group

Even if they are exactly the same, it seems like we may not want an alias because we will want to deprecate the ONEAPI version in favor of the standard one. Therefore, it seems like defining separate types is the right thing.

@vladimirlaz
Copy link

It looks like the requested change should be applied to RT headers.
The tests are accurate (interfaces from SYCL::ONEAPI namespace are used).
@Pennycook, @AlexeySachkov could you approve the PR if no objections?

@maximdimakov
Copy link
Author

Providing ONEAPI aliases for function objects is needed for intel/llvm#3868

@maximdimakov maximdimakov changed the title [SYCL] Specified ONEAPI aliases in functions [SYCL] Specified ONEAPI aliases in functions and function objects Jun 3, 2021
Copy link

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

I'm not opposed to rewriting the existing tests to be more explicit in their use of namespaces.

@vladimirlaz vladimirlaz merged commit 0d6e673 into intel:intel Jun 4, 2021
smaslov-intel pushed a commit to smaslov-intel/llvm-test-suite that referenced this pull request Aug 12, 2021
…tel#300)

* [SYCL] Added test for constexpr half operations

Signed-off-by: mdimakov <maxim.dimakov@intel.com>
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
…tel/llvm-test-suite#300)

* [SYCL] Added test for constexpr half operations

Signed-off-by: mdimakov <maxim.dimakov@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants