[SYCL][ABI Break] Remove "cl" namespace #6518

aelovikov-intel · 2022-08-02T18:48:34Z

As part of the change, also start using "inline namespace _V1" to allow
possible future ABI-affecting changes.

aelovikov-intel · 2022-08-02T20:37:18Z

SPIRV upstream change: KhronosGroup/SPIRV-LLVM-Translator#1564

As part of the change, also start using "inline namespace _V1" to allow possible future ABI-affecting changes.

llvm-spirv/lib/SPIRV/SPIRVUtil.cpp

aelovikov-intel · 2022-08-03T18:03:44Z

sycl/gdb/libsycl.so-gdb.py

@@ -104,7 +104,7 @@ def __init__(self, class_type, result_type, depth):
        self.depth = depth

    def get_arg_types(self):
-        return gdb.lookup_type("cl::sycl::id<%s>" % self.depth)
+        return gdb.lookup_type("sycl::_V1::id<%s>" % self.depth)


Not sure how this file is being tested, so the changes here are "blind".

@stevemerr, do you know?

I suspect any end-to-end testing that would run gdb-oneapi on a sycl sample would exercise this file.

Does this need to be sycl::_V1::id or can we just do sycl::id?

This PRs makes it sycl::_V1::id to give us a chance to avoid ABI-breakage in the future by switching to sycl::_V2::id while preserving the binary impl for _V1.

aelovikov-intel · 2022-08-03T18:57:39Z

/verify with intel/llvm-test-suite#1128

premanandrao

FE changes look good to me.

smanna12

FE changes OK to me.

asudarsa · 2022-08-04T20:46:07Z

llvm-spirv/lib/SPIRV/SPIRVUtil.cpp

@@ -303,7 +303,7 @@ bool isSYCLBfloat16Type(llvm::Type *Ty) {
      return false;
    StringRef Name = ST->getName();
    Name.consume_front("class.");
-    if ((Name.startswith("cl::sycl::") ||
+    if ((Name.startswith("cl::sycl::") || Name.startswith("sycl::_V1::") ||


This is mostly my ignorance. can you please outline why we used sycl::_V1 here and just sycl earlier? Also, in the upstream change, I see only sycl here. Thanks

This PR makes two independent changes at once, only because decoupling them would be lots of useless work with little benefit.

First change is to get rid of cl:: and put sycl directly into the global namespace.

The second part is groundwork for possible future ABI breaking changes. The idea is that if we'd need, for example, make ABI breaking changes into sycl::queue, we'll do something like this:

// sycl.hpp: namespace sycl { inline namespace _V2 { class queue {}; // That's what new code compiled with new compiler/RT would refer to by sycl::queue } } // sycl_queue_v1_compat.cpp, linked into future libsycl.so namespace sycl { namespace _V1 { class queue { ... }; // Old binaries linked with new sycl.so would use this. // Old implementation } }

The exact machinery for that is to be defined later when/if we'd actually need it. However, we need to introduce _V right now because if our current queue would be sycl::queue instead of sycl::_V1::queue (that's how inline namespace is being mangled) we won't have any possibility to implement the sketchy solution above.

Actually, I probably misunderstood the question. The proper fix upstream is using ^sycl::, without _V1 so that it would work if/when we'll start using sycl::_V2 symbols.

The final version will be what upstream has.

Thanks for the explanation Andrei. I understand now. I think this is good as long as it is documented somewhere.
I can approve the SPIRV changes. Thanks for this.

I'm not sure I follow what problem llvm-spirv translator changes solves. What will happen when runtime will switch to sycl::_V2 symbols? Translator will leave bfloat16 types as user defined types instead of SPIR-V buillt-in types. Right?

In general, please, avoid making local changes to the translator. All changes should be committed to the Khronos repository. Ideally we should remove the copy of sources from our repository and fetch them directly from the Khronos repository.

asudarsa

LGTM

bader · 2022-08-07T08:42:39Z

clang/lib/CodeGen/CGSYCLRuntime.cpp

  static std::array<Util::DeclContextDesc, 3> Scopes = {
-      Util::DeclContextDesc{clang::Decl::Kind::Namespace, "cl"},
      Util::DeclContextDesc{clang::Decl::Kind::Namespace, "sycl"},
+      Util::DeclContextDesc{clang::Decl::Kind::Namespace, "_V1"},


Do you we have SYCL 1.2.1 compatibility tests?
https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:headers-and-namespaces
We should have them in SYCL-CTS and make sure that DPC++ complies.

Are we talking source-compatible or binary-compatible? The former is being done with namespace alias as part of this PR (I might need to add an explicit test for that though), while the latter shouldn't (can't?) be done.

I think we are talking about source-compatibility. Yes, please, add SYCL-1.2.1 compatibility test to make sure that when <CL/sycl.hpp> is included, we can still use data types defined in cl::sycl namespace in device code.

I think we would need to account for both scopes (here and in Sema) for compatibility right?

We need to remove these name based checks. Now that we're modifying namespaces, this isn't future proof at all. We should instead modify sycl_special attribute to accept an argument and use that to do these checks where required.

I think we would need to account for both scopes (here and in Sema) for compatibility right?

I don't think we do, but I'd like to hear your reasoning why you think we might need that.

We need to remove these name based checks. ... We should instead modify sycl_special attribute...

That is way above my comfort level with the FE - I guess I'd need someone from FE to make that part of the change.
Also, IMO, it should be done in a separate patch (either before or after this PR).

I don't think we do, but I'd like to hear your reasoning why you think we might need that.

If the cl/sycl header is used as @bader mentioned above and users declare types using cl::sycl::aspect, etc, wouldn't compiler checks like isDeviceAspectType fail since it checks hard-coded scope which is now sycl::_V1::aspect?

That is way above my comfort level with the FE - I guess I'd need someone from FE to make that part of the change.
Also, IMO, it should be done in a separate patch (either before or after this PR).

I can help with that. What is the timeline for this change like?

Using cl/sycl.h doesn't change ABI, only introduce namespace alias (https://godbolt.org/z/x17Toq5vY).

I can help with that. What is the timeline for this change like?

I don't think there are hard requirement other than the 2023 release, but this PR gets merge conflicts pretty easily and I really wouldn't want it to sit uncommitted for long.

Ok Thanks for explaining. I guess this should work then, as long as we don't use older versions of cl/sycl.h

kbobrovs

ESIMD part LGTM

steffenlarsen

Some minor comments, otherwise the runtime and the device headers look good.

sycl/source/detail/builtins_common.cpp

sycl/source/ld-version-script.txt

The interfaces are simple and *very* unlikely to change. Also, changing _V1::cl_float to _V2::cl_float would work without versioning of the routines in __host_std as the arguments' types are part of the mangling already.

aelovikov-intel · 2022-08-10T19:26:51Z

/verify with intel/llvm-test-suite#1128

smanna12

FE changes look good to me.

asudarsa

SPIR-V changes look good. Thanks.

aelovikov-intel · 2022-08-10T21:21:37Z

/verify with intel/llvm-test-suite#1128

aelovikov-intel · 2022-08-10T21:23:22Z

Given how easy it is to get into a merge conflict and how many approvals are required (and hard to get quickly enough), I don't think it will ever be green...

@intel/llvm-gatekeepers , should we weaken the merge criteria here maybe?

v-klochkov · 2022-08-10T21:40:06Z

Given how easy it is to get into a merge conflict and how many approvals are required (and hard to get quickly enough), I don't think it will ever be green...

@intel/llvm-gatekeepers , should we weaken the merge criteria here maybe?

If your changes only remove "cl", then you have pre-approval from ESIMD (no need for explicit approval here). Otherwise, please ping @intel/dpcpp-esimd-reviewers

aelovikov-intel · 2022-08-10T21:45:53Z

If your changes only remove "cl", then you have pre-approval from ESIMD (no need for explicit approval here). Otherwise, please ping https://github.com/orgs/intel/teams/dpcpp-esimd-reviewers

They also contain corresponding changes in places where mangling matters, like ESIMDVerifier.cpp/LowerESIMD.cpp.

aelovikov-intel · 2022-08-11T16:47:15Z

/verify with intel/llvm-test-suite#1128

aelovikov-intel · 2022-08-11T18:42:55Z

Failures on LLVM Test Suite precommit are expected and expected to pass in "verify with" testing. An exception is

Timed Out Tests (1):
  SYCL :: HierPar/hier_par_basic.cpp

which is most likely unrelated. I'd argue that (pending Jenkins/llvm-test-suite) we should proceed with merging this. If, in an unlikely event, of the failure being real I'll address it post-commit.

Edit: Jenkins/precomit is clear. I believe this is as much ready for the merge as would ever be possible, despite missing some formal review approval statuses. @intel/llvm-gatekeepers would you agree to merge this in?

aelovikov-intel force-pushed the ns-cl branch 2 times, most recently from 2336f6e to 75ef986 Compare August 2, 2022 20:00

aelovikov-intel force-pushed the ns-cl branch from 75ef986 to 2008354 Compare August 2, 2022 21:58

[SYCL][ABI Break] Remove "cl" namespace

ce49e08

As part of the change, also start using "inline namespace _V1" to allow possible future ABI-affecting changes.

aelovikov-intel force-pushed the ns-cl branch from 2008354 to ce49e08 Compare August 3, 2022 17:58

aelovikov-intel mentioned this pull request Aug 3, 2022

[SYCL] Complementary change for removing cl:: namespace intel/llvm-test-suite#1128

Merged

aelovikov-intel marked this pull request as ready for review August 3, 2022 17:59

aelovikov-intel requested review from a team as code owners August 3, 2022 17:59

aelovikov-intel requested a review from smaslov-intel August 3, 2022 17:59

aelovikov-intel commented Aug 3, 2022

View reviewed changes

llvm-spirv/lib/SPIRV/SPIRVUtil.cpp Outdated Show resolved Hide resolved

aelovikov-intel commented Aug 3, 2022

View reviewed changes

premanandrao previously approved these changes Aug 4, 2022

View reviewed changes

smanna12 previously approved these changes Aug 4, 2022

View reviewed changes

asudarsa reviewed Aug 4, 2022

View reviewed changes

asudarsa previously approved these changes Aug 4, 2022

View reviewed changes

bader reviewed Aug 7, 2022

View reviewed changes

bader mentioned this pull request Aug 7, 2022

[SYCL][SPIR-V] Change the LLVM type name of SPIR-V matrix types. #6535

Merged

kbobrovs previously approved these changes Aug 8, 2022

View reviewed changes

steffenlarsen reviewed Aug 8, 2022

View reviewed changes

sycl/source/detail/builtins_common.cpp Outdated Show resolved Hide resolved

sycl/source/ld-version-script.txt Outdated Show resolved Hide resolved

aelovikov-intel added 3 commits August 8, 2022 12:28

Merge remote-tracking branch 'origin/sycl' into ns-cl

64d972a

Update a comment per code review

4e54eb1

Remove TODOs regarding __host_std versioning

678f522

The interfaces are simple and *very* unlikely to change. Also, changing _V1::cl_float to _V2::cl_float would work without versioning of the routines in __host_std as the arguments' types are part of the mangling already.

Add a basic test

eaf3b39

aelovikov-intel dismissed elizabethandrews’s stale review via eaf3b39 August 10, 2022 19:26

smanna12 previously approved these changes Aug 10, 2022

View reviewed changes

steffenlarsen previously approved these changes Aug 10, 2022

View reviewed changes

asudarsa previously approved these changes Aug 10, 2022

View reviewed changes

Merge branch 'sycl' into ns-cl

930fecd

aelovikov-intel dismissed stale reviews from asudarsa, steffenlarsen, and smanna12 via 930fecd August 10, 2022 21:21

v-klochkov previously approved these changes Aug 10, 2022

View reviewed changes

elizabethandrews previously approved these changes Aug 10, 2022

View reviewed changes

steffenlarsen previously approved these changes Aug 10, 2022

View reviewed changes

smanna12 previously approved these changes Aug 10, 2022

View reviewed changes

Merge remote-tracking branch 'origin/sycl' into ns-cl

1265739

aelovikov-intel dismissed stale reviews from smanna12, steffenlarsen, elizabethandrews, and v-klochkov via 1265739 August 11, 2022 16:44

steffenlarsen approved these changes Aug 11, 2022

View reviewed changes

smanna12 approved these changes Aug 11, 2022

View reviewed changes

steffenlarsen merged commit 3d2b25e into intel:sycl Aug 11, 2022

aelovikov-intel mentioned this pull request Aug 13, 2022

[NFC][SYCL] Remove explicit cl:: namespace usage #6485

Merged

aelovikov-intel deleted the ns-cl branch August 25, 2022 20:10

[SYCL][ABI Break] Remove "cl" namespace #6518

[SYCL][ABI Break] Remove "cl" namespace #6518

Uh oh!

Conversation

aelovikov-intel commented Aug 2, 2022

Uh oh!

aelovikov-intel commented Aug 2, 2022

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!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel commented Aug 3, 2022

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

asudarsa Aug 4, 2022 • 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!

aelovikov-intel Aug 4, 2022 • 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!

asudarsa left a comment

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!

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Aug 9, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aelovikov-intel commented Aug 10, 2022

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

asudarsa Aug 4, 2022 •

edited

Loading

aelovikov-intel Aug 4, 2022 •

edited

Loading

elizabethandrews Aug 9, 2022 •

edited

Loading

aelovikov-intel commented Aug 11, 2022 •

edited

Loading