[SYCL] Refactor cl::sycl namespace. Part1 #4397

romanovvlad · 2021-08-24T19:13:53Z

The patch is the first in series of patches that implement support for
SYCL2020 namespace requirements - sycl:: instead of cl::sycl.

The patch replaces explicit uses of cl::sycl namespace with a configurable
macro which will be used in future patches.

romanovvlad · 2021-08-24T19:16:14Z

@erichkeane Could you please take a look at SemaSYCL.cpp modifications? The patch replaces explicit usages of cl and sycl namespaces openings with macro which are defined in defines_elementary.hpp.

erichkeane · 2021-08-24T19:17:51Z

Clang changes so far are seemingly fine. You might consider removing one set of '{' and '}' from it though so that the source looks something like:

__SYCL_OPEN_NS {

....
} __SYCL_CLOSE_NS

Or something. It would at least make the scoping a little easier to read.

sycl/.clang-format

bader

I've reviewed half of the changed files and left a few comments.

Could you clarify how many parts of the refactoring is going to be and give an overview of the changes in other parts, please?

sycl/include/CL/sycl/detail/generic_type_lists.hpp

sycl/include/CL/sycl/detail/locked.hpp

clang/test/CodeGenSYCL/kernel-param-acc-array-ih.cpp

sycl/include/CL/sycl/atomic.hpp

sycl/include/CL/sycl/detail/common.hpp

sycl/include/CL/sycl/device.hpp

sycl/include/CL/sycl/kernel_bundle_enums.hpp

sycl/include/CL/sycl/pipes.hpp

sycl/include/sycl/ext/intel/experimental/esimd/detail/host_util.hpp

romanovvlad · 2021-08-30T14:23:49Z

I've reviewed half of the changed files and left a few comments.

Could you clarify how many parts of the refactoring is going to be and give an overview of the changes in other parts, please?

I see it like this:

Introduce namespace macro(this patch)
Add aliases from the old ABI to the new one + switch to new namespaces by default
Move files from CL/sycl to sycl
Bugfixes if any

bader

Comments for the second half of the files.

sycl/include/sycl/ext/oneapi/atomic_ref.hpp

bader · 2021-08-30T16:41:07Z

sycl/include/CL/sycl/detail/defines_elementary.hpp

+
+#ifdef __SYCL_ENABLE_SYCL121_NAMESPACE
+// Old SYCL1.2.1 namespace scheme
+#define __SYCL_NS_OPEN_1 __SYCL_INLINE_NAMESPACE(cl)


Can we remove __SYCL_INLINE_NAMESPACE? I think it's not supposed to be used explicitly anymore. Right?

It is still used for builtins declaration - https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/detail/builtins.hpp#L106 and will be used until we remove support for old namespaces.

Will get rid of this macro in the second patch(by introducing a new one).

It is still used for builtins declaration - https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/detail/builtins.hpp#L106 and will be used until we remove support for old namespaces.

#ifndef __SYCL_DEVICE_ONLY__ __SYCL_INLINE_NAMESPACE(cl) { namespace __host_std {

I think we should re-visit the design of this code.
It is unclear to me why we put built-in declarations to cl namespace. Another question: why do we inline cl namespace here? I understand that it's needed to allow users to use sycl namespace w/o cl:: qualifier, but __host_std namespace should not be exposed like this. Right?

Will get rid of this macro in the second patch(by introducing a new one).

Could you clarify what new macro is for?

If this is the only use, can we just use inline namespace instead of the macro here? I'm okay to make this change in follow-up patches.

sycl/test/basic_tests/test_new_namespace.cpp

sycl/source/ld-version-script.txt

MrSidims

FPGA file changes are LGTM

romanovvlad · 2021-08-31T11:41:46Z

bfloat16 fail should be fixed by #4388

premanandrao

FE changes LGTM

romanovvlad · 2021-08-31T15:13:50Z

clang/lib/Sema/SemaSYCL.cpp

@@ -4788,8 +4788,7 @@ void SYCLIntegrationHeader::emit(raw_ostream &O) {
      FwdDeclEmitter.Visit(K.NameType);
  O << "\n";

-  O << "__SYCL_INLINE_NAMESPACE(cl) {\n";
-  O << "namespace sycl {\n";


Found a problem with this change.

What is the problem?

The problem is that the integration header includes defines_elementary.hpp which selects either SYCL 1.2.1 of SYCL2020 style of namespaces before we know which one should be used. So, it seems it was a bad idea to use the same macro in the integration header and for headers. Going to introduce a separate macro for integration header.

@erichkeane @premanandrao Does it sound OK?

That sounds like that should be alright... we just have to make sure the two stay in sync. Though, I'm not sure how you can get a macro here to work if it depends on a decision that isn't made yet. Should defines_elementary.hpp be doing all of the lifting for this set of namespaces?

Can we just use inline namespace cl w/o any macro defined in DPC++ headers?

That sounds like that should be alright... we just have to make sure the two stay in sync.
Though, I'm not sure how you can get a macro here to work if it depends on a decision that isn't made yet.
Should defines_elementary.hpp be doing all of the lifting for this set of namespaces?

I'm thinking about separating those namespaces. For integration header we could have a macro in the defines_elementary.hpp which, for example, defines to __sycl_int_header_ns (it's not quite suitable for the integration footer though). For headers we could you an additional header(defines_sycl_ns.hpp) which is not included from the integration header - when we include it we know which namespace version we should use.

Can we just use inline namespace cl w/o any macro defined in DPC++ headers?

We can, but we cannot use inline namespace cl, because user space will be still poluted. We can use a different namespace - __sycl_int_header_ns, but with a macro it would be easier to be in sync without FE modifications.

bader · 2021-08-31T15:15:25Z

I've reviewed half of the changed files and left a few comments.
Could you clarify how many parts of the refactoring is going to be and give an overview of the changes in other parts, please?

I see it like this:

Introduce namespace macro(this patch)

Add aliases from the old ABI to the new one + switch to new namespaces by default

Move files from CL/sycl to sycl

Bugfixes if any

Please, update the PR title to something like "[SYCL] Refactor cl::sycl namespace [part 1 of 3]." and add the details from your comment to the description.

romanovvlad added 5 commits August 24, 2021 21:50

Start namespace renaming

c70e525

clang formatting

813b217

minor Fixes

535e32e

tiny fixes

ffbead5

Add comments and test

462f4b1

romanovvlad requested review from alexbatashev and erichkeane August 24, 2021 19:13

romanovvlad added 2 commits August 30, 2021 13:15

Apply comments and fix tests

358994c

Merge remote-tracking branch 'public/sycl' into private2/vromanov/Alias

37b945e

romanovvlad marked this pull request as ready for review August 30, 2021 10:29

romanovvlad requested review from AaronBallman, againull, AlexeySachkov, DenisBakhvalov, elizabethandrews, kbobrovs, kychendev, MrSidims, Pennycook, premanandrao, smaslov-intel, turinevgeny and a team as code owners August 30, 2021 10:29

alexbatashev previously approved these changes Aug 30, 2021

View reviewed changes

sycl/.clang-format Show resolved Hide resolved

romanovvlad added 2 commits August 30, 2021 14:13

clang formatting

1fb1ec8

more clang formatting

c2d6b58

romanovvlad dismissed alexbatashev’s stale review via c2d6b58 August 30, 2021 12:48

romanovvlad requested a review from bader as a code owner August 30, 2021 12:48

romanovvlad requested review from pvchupin and a team as code owners August 30, 2021 12:48

alexbatashev previously approved these changes Aug 30, 2021

View reviewed changes

bader reviewed Aug 30, 2021

View reviewed changes

romanovvlad added 2 commits August 30, 2021 17:16

resolve comments

911d4d1

resolve comments

b24a5c0

romanovvlad dismissed alexbatashev’s stale review via b24a5c0 August 30, 2021 14:27

smaslov-intel previously approved these changes Aug 30, 2021

View reviewed changes

bader reviewed Aug 30, 2021

View reviewed changes

MrSidims previously approved these changes Aug 31, 2021

View reviewed changes

romanovvlad mentioned this pull request Aug 31, 2021

There must be check for __SYCL_CLOSE_NS() after the checks for kernel_names, kernel_signatures and KernelInfo. #4437

Closed

romanovvlad added 2 commits August 31, 2021 12:31

address comments

24ee4ec

Merge remote-tracking branch 'public/sycl' into private2/vromanov/Alias

53d3767

romanovvlad dismissed stale reviews from MrSidims and smaslov-intel via 53d3767 August 31, 2021 09:41

romanovvlad requested a review from bader August 31, 2021 09:42

premanandrao approved these changes Aug 31, 2021

View reviewed changes

turinevgeny approved these changes Aug 31, 2021

View reviewed changes

bader approved these changes Aug 31, 2021

View reviewed changes

romanovvlad commented Aug 31, 2021

View reviewed changes

romanovvlad closed this Sep 9, 2021

[SYCL] Refactor cl::sycl namespace. Part1 #4397

[SYCL] Refactor cl::sycl namespace. Part1 #4397

Uh oh!

Conversation

romanovvlad commented Aug 24, 2021

Uh oh!

romanovvlad commented Aug 24, 2021

Uh oh!

erichkeane commented Aug 24, 2021

Uh oh!

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romanovvlad commented Aug 30, 2021

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

romanovvlad commented Aug 31, 2021

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romanovvlad Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader commented Aug 31, 2021

Uh oh!

Uh oh!

bader Aug 31, 2021 •

edited

Loading

romanovvlad Aug 31, 2021 •

edited

Loading