[SYCL] Move general language extensions to the ONEAPI namespace #2231

jbrodman · 2020-07-31T14:43:25Z

Breaking #2155 into smaller PRs.

This concentrates general language extensions into the ONEAPI namespace.

It includes:
Atomics
Printf
Group Algorithms
Reductions
Sub-groups
Specialization Constants
Function Pointers

Signed-off-by: James Brodman james.brodman@intel.com

Signed-off-by: James Brodman <james.brodman@intel.com>

Pennycook

I'd be okay with changes to documentation being addressed in a separate PR, but I'm curious to hear what others think.

Another concern I have is that this will appear as a breaking change between one compiler release and the next, with no warning. Would it be worth adding something temporary defining the intel namespace as containing everything from the ONEAPI namespace?

Pennycook · 2020-07-31T15:06:23Z

clang/test/CodeGenSYCL/Inputs/sycl.hpp

@@ -242,7 +242,7 @@ struct get_kernel_name_t<auto_name, Type> {
  using name = Type;
 };

-namespace experimental {
+namespace ONEAPI {


intel::experimental::spec_constant doesn't have an extension defined, and I think we might confuse developers if we start mixing extensions and non-extensions in the same namespace. As is, using sycl::ONEAPI will import experimental functionality unexpectedly.

Is there a reason not to keep this as ONEAPI::experimental after the rename? Otherwise I think somebody should define extensions for these things.

Same comment applies to intel::experimental::printf.

Ok. I'd prefer to make it ONEAPI::experimental over intel::experimental.

Pennycook · 2020-07-31T15:09:00Z

sycl/include/CL/sycl/ONEAPI/atomic.hpp

@@ -1,4 +1,4 @@
-//==---------------- atomic.hpp - SYCL_INTEL_extended_atomics --------------==//
+//==--------------- atomic.hpp - SYCL_ONEAPI_extended_atomics --------------==//


If we're changing the name of the extensions as well as the namespace they're in, I think we should really update the extension documentation.

Ok sure - but can that be a different PR?

Fine by me, but I wanted to flag it somewhere.

Pennycook · 2020-07-31T15:11:09Z

sycl/include/CL/sycl/ONEAPI/group_algorithm.hpp


-#ifndef __DISABLE_SYCL_INTEL_GROUP_ALGORITHMS__
+#ifndef __DISABLE_SYCL_ONEAPI_GROUP_ALGORITHMS__


Same comment as before RE naming. This macro is now undocumented.

alexbatashev

@jbrodman you need to update DEV_ABI version in CMakeLists.txt

BTW, any particular reason why it's spelled ONEAPI and not oneapi?

keryell · 2020-08-03T15:46:05Z

@jbrodman you need to update DEV_ABI version in CMakeLists.txt

BTW, any particular reason why it's spelled ONEAPI and not oneapi?

I was about to suggest... oneAPI. :-)
The recent feedback from SYCL committee was to use uppercase for extensions to limit conflict risks. But perhaps it should be at least 1 uppercase letter? So we can have also triSYCL, hipSYCL, ComputeCpp...

Signed-off-by: James Brodman <james.brodman@intel.com>

bader

I think I'm owning only one file - llvm/test/tools/sycl-post-link/sc_sym_two_refs.ll.
Changes in this file looks okay, but doesn't seem to be necessary for runtime changes.

llvm/test/tools/sycl-post-link/sc_sym_two_refs.ll

Signed-off-by: James Brodman <james.brodman@intel.com>

Fznamznon

I'm ok with FE changes.

Pennycook

I didn't spot this before because it isn't part of the diff, but atomic_accessor.hpp still has _INTEL_ in its copyright header. It might be worth grepping to see if any others do as well.

Signed-off-by: James Brodman <james.brodman@intel.com>

jbrodman · 2020-08-04T17:22:37Z

I didn't spot this before because it isn't part of the diff, but atomic_accessor.hpp still has _INTEL_ in its copyright header. It might be worth grepping to see if any others do as well.

Fixed. Didn't find more.

bader · 2020-08-17T16:41:46Z

/summary:run

jbrodman · 2020-08-18T14:44:29Z

Ping @Pennycook @AlexeySachkov @kbobrovs @premanandrao @elizabethandrews

Signed-off-by: James Brodman <james.brodman@intel.com>

This resolves #2224 and fixes #2229 Original commit: KhronosGroup/SPIRV-LLVM-Translator@10d1285

Move general language extensions to the ONEAPI namespace

e0b8033

Signed-off-by: James Brodman <james.brodman@intel.com>

jbrodman requested review from AlexeySachkov, elizabethandrews, Fznamznon, kbobrovs, Pennycook, premanandrao, v-klochkov and a team as code owners July 31, 2020 14:43

Pennycook reviewed Jul 31, 2020

View reviewed changes

alexbatashev reviewed Aug 3, 2020

View reviewed changes

jbrodman added 2 commits August 3, 2020 13:07

Move spec constants and printf to ONEAPI::experimental

56bbbfa

Signed-off-by: James Brodman <james.brodman@intel.com>

Merge branch 'sycl' into oneapins

16dbb8d

Signed-off-by: James Brodman <james.brodman@intel.com>

jbrodman requested a review from bader as a code owner August 3, 2020 17:10

jbrodman added 3 commits August 3, 2020 13:55

clang-format

42c6c27

Signed-off-by: James Brodman <james.brodman@intel.com>

Fix include hell and a dangling namespace

c7ce1d5

Signed-off-by: James Brodman <james.brodman@intel.com>

clang-format

bd26c21

Signed-off-by: James Brodman <james.brodman@intel.com>

bader previously approved these changes Aug 3, 2020

View reviewed changes

llvm/test/tools/sycl-post-link/sc_sym_two_refs.ll Outdated Show resolved Hide resolved

llvm/test/tools/sycl-post-link/sc_sym_two_refs.ll Outdated Show resolved Hide resolved

AlexeyB var name simplification

6308c5d

Signed-off-by: James Brodman <james.brodman@intel.com>

jbrodman dismissed bader’s stale review via 6308c5d August 3, 2020 18:50

jbrodman changed the title ~~[SYCL][WIP] Move general language extensions to the ONEAPI namespace~~ [SYCL] Move general language extensions to the ONEAPI namespace Aug 3, 2020

update dangling ns in test and clang-format

6207521

Signed-off-by: James Brodman <james.brodman@intel.com>

Fznamznon previously approved these changes Aug 4, 2020

View reviewed changes

jbrodman requested review from Fznamznon and Pennycook August 4, 2020 14:40

Pennycook requested changes Aug 4, 2020

View reviewed changes

Update comment and bump ABI version

2fe18fe

Signed-off-by: James Brodman <james.brodman@intel.com>

jbrodman dismissed Fznamznon’s stale review via 2fe18fe August 4, 2020 16:55

jbrodman dismissed Pennycook’s stale review via 4877aad August 17, 2020 14:20

jbrodman requested review from bader, Pennycook, v-klochkov and Fznamznon August 17, 2020 16:04

Fznamznon previously approved these changes Aug 17, 2020

View reviewed changes

bader previously approved these changes Aug 17, 2020

View reviewed changes

v-klochkov previously approved these changes Aug 17, 2020

View reviewed changes

jbrodman requested a review from alexbatashev August 18, 2020 14:43

Pennycook previously approved these changes Aug 18, 2020

View reviewed changes

AlexeySachkov previously approved these changes Aug 19, 2020

View reviewed changes

Merge branch 'sycl' into oneapins

f46ef3b

Signed-off-by: James Brodman <james.brodman@intel.com>

jbrodman dismissed stale reviews from AlexeySachkov, Pennycook, v-klochkov, bader, and Fznamznon via f46ef3b August 20, 2020 14:46

bader approved these changes Aug 20, 2020

View reviewed changes

elizabethandrews approved these changes Aug 20, 2020

View reviewed changes

premanandrao approved these changes Aug 20, 2020

View reviewed changes

bader requested review from AlexeySachkov, Pennycook and v-klochkov August 20, 2020 15:58

AlexeySachkov approved these changes Aug 20, 2020

View reviewed changes

Pennycook approved these changes Aug 20, 2020

View reviewed changes

bader merged commit a73369d into intel:sycl Aug 20, 2020

AlexeySachkov mentioned this pull request Dec 18, 2020

Rename sycl::INTEL::gpu to sycl::intel::gpu #2903

Closed

jsji pushed a commit that referenced this pull request Nov 30, 2023

Fix translation for static TypeMember for dwarf5 (#2231)

4b4b6d6

This resolves #2224 and fixes #2229 Original commit: KhronosGroup/SPIRV-LLVM-Translator@10d1285

		@@ -1,4 +1,4 @@
		//==---------------- atomic.hpp - SYCL_INTEL_extended_atomics --------------==//
		//==--------------- atomic.hpp - SYCL_ONEAPI_extended_atomics --------------==//


		#ifndef __DISABLE_SYCL_INTEL_GROUP_ALGORITHMS__
		#ifndef __DISABLE_SYCL_ONEAPI_GROUP_ALGORITHMS__

[SYCL] Move general language extensions to the ONEAPI namespace #2231

[SYCL] Move general language extensions to the ONEAPI namespace #2231

Uh oh!

Conversation

jbrodman commented Jul 31, 2020

Uh oh!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

Pennycook Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

jbrodman Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

Pennycook Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

jbrodman Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

Pennycook Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

Pennycook Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

alexbatashev left a comment

Choose a reason for hiding this comment

Uh oh!

keryell commented Aug 3, 2020

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

jbrodman commented Aug 4, 2020

Uh oh!

bader commented Aug 17, 2020

Uh oh!

jbrodman commented Aug 18, 2020

Uh oh!

Uh oh!