[SYCL] Replace hardcoded namespaces with attribute #6674

elizabethandrews · 2022-08-30T22:14:02Z

Namespaces were hardcoded and used in compiler to
check for various SYCL types including accessors,
spec_constants, etc. This patch implements an
attribute to uniquely identify the types instead.
Attribute argument is an Identifier which denotes
each type.

E.g. attribute((sycl_type(accessor)) is used
to mark accessor class.

The attribute has been implemented as with an
accepted list of arguments via EnumArg. The attribute
definition should be updated to support any new types.

The attribute takes 1 argument.

Fixes: #5186

Namespaces were hardcoded and used in compiler to check for various SYCL types including accessors, spec_constants, etc. This patch implements an attribute to uniquely identify the types instead. Attribute argument is an Identifier which denotes each type. E.g. __attribute__((sycl_type(accessor)) is used to mark accessor class. The attribite has been implemented as with an accepted list of arguments via EnumArg. The attribute definition should be updated to support any new types. The attribute takes 1 argument. Signed-off-by: Elizabeth Andrews <[email protected]>

Signed-off-by: Elizabeth Andrews <[email protected]>

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Sema/SemaSYCL.cpp

clang/include/clang/Basic/Attr.td

Fznamznon

I believe this one will fix #5186 .

clang/include/clang/Basic/Attr.td

clang/lib/Sema/SemaSYCL.cpp

clang/test/SemaSYCL/sycl-type-attr-ast.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

Fznamznon

Just a couple of nits to the tests, otherwise LGTM.

clang/test/SemaSYCL/sycl-type-attr.cpp

clang/test/SemaSYCL/sycl-type-attr-ast.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

clang/test/SemaSYCL/sycl-type-attr.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

premanandrao

Just nits, except in one place. Otherwise, looks good.

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Elizabeth Andrews <[email protected]>

steffenlarsen

LGTM! Great improvement!

For the future, should we consider removing __sycl_detail__::device_global in favor of a device_global enum for __sycl_detail__::sycl_type?

elizabethandrews · 2022-09-01T17:10:44Z

LGTM! Great improvement!

For the future, should we consider removing __sycl_detail__::device_global in favor of a device_global enum for __sycl_detail__::sycl_type?

I don't see why not. I can do that in a follow-up PR.

elizabethandrews · 2022-09-01T17:25:32Z

The failing CUDA test suite looks like an infrastructure issue.

elizabethandrews · 2022-09-01T18:54:54Z

The failing CUDA test suite looks like an infrastructure issue.

I see the following test failing -

TEST 'SYCL :: Plugin/level_zero_events_caching.cpp

Is this a known issue? I haven't looked carefully at this but it may not be related to my patch because all tests passed for the first patch I pushed, and none of the subsequent changes should introduce new failures

steffenlarsen · 2022-09-01T19:01:22Z

The failing CUDA test suite looks like an infrastructure issue.

I see the following test failing -

TEST 'SYCL :: Plugin/level_zero_events_caching.cpp

Is this a known issue? I haven't looked carefully at this but it may not be related to my patch because all tests passed for the first patch I pushed, and none of the subsequent changes should introduce new failures

I haven't seen that failure before. @againull | @smaslov-intel - Is this a known failure?

bader

Thanks!
I still think that it's possible to come up with unified interface to handle all SYCL classes the same way, so we should be able to drop attribute argument in the future.

elizabethandrews · 2022-09-06T15:06:16Z

@intel/llvm-gatekeepers I don't believe the fails have anything to do with this PR. CUDA suite seems to be having some infrastructure issues. SYCL :: DeviceLib/assert.cpp did not fail when I ran the test on earlier. There have been no new commits/changes since then in this PR

steffenlarsen · 2022-09-06T18:03:21Z

Failing OCL x64 was a known issue and has since been fixed. CUDA failure does indeed look to be infrastructure-related.

elizabethandrews added 3 commits August 30, 2022 09:17

Merge remote-tracking branch 'intel_llvm/sycl' into syclTypeAttr

60ec6f1

[SYCL] Add changes for local_accessor

839c3ff

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews requested review from a team as code owners August 30, 2022 22:14

elizabethandrews requested a review from steffenlarsen August 30, 2022 22:14

AlexeySachkov reviewed Aug 31, 2022

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Show resolved Hide resolved

AlexeySachkov requested a review from KornevNikita August 31, 2022 10:15

steffenlarsen reviewed Aug 31, 2022

View reviewed changes

clang/include/clang/Basic/Attr.td Outdated Show resolved Hide resolved

Fznamznon reviewed Aug 31, 2022

View reviewed changes

clang/include/clang/Basic/Attr.td Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/test/SemaSYCL/sycl-type-attr-ast.cpp Show resolved Hide resolved

Apply review comments

d4e97ab

Signed-off-by: Elizabeth Andrews <[email protected]>

Fznamznon reviewed Sep 1, 2022

View reviewed changes

clang/test/SemaSYCL/sycl-type-attr.cpp Outdated Show resolved Hide resolved

clang/test/SemaSYCL/sycl-type-attr-ast.cpp Outdated Show resolved Hide resolved

Remove unnecessary includes and options from tests

ddbec09

Signed-off-by: Elizabeth Andrews <[email protected]>

Fznamznon reviewed Sep 1, 2022

View reviewed changes

clang/test/SemaSYCL/sycl-type-attr.cpp Outdated Show resolved Hide resolved

Delete unused option in test RUN

0418fea

Signed-off-by: Elizabeth Andrews <[email protected]>

Fznamznon previously approved these changes Sep 1, 2022

View reviewed changes

premanandrao reviewed Sep 1, 2022

View reviewed changes

Implement review comments

cca9a85

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews dismissed Fznamznon’s stale review via cca9a85 September 1, 2022 16:41

steffenlarsen approved these changes Sep 1, 2022

View reviewed changes

Fznamznon approved these changes Sep 1, 2022

View reviewed changes

premanandrao approved these changes Sep 1, 2022

View reviewed changes

smanna12 approved these changes Sep 2, 2022

View reviewed changes

bader approved these changes Sep 5, 2022

View reviewed changes

steffenlarsen merged commit 77dd693 into intel:sycl Sep 6, 2022

elizabethandrews mentioned this pull request Jan 17, 2023

[SYCL] Implement host pipe unique name generation and mapping calls #8009

Merged

[SYCL] Replace hardcoded namespaces with attribute #6674

[SYCL] Replace hardcoded namespaces with attribute #6674

Uh oh!

Conversation

elizabethandrews commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews commented Sep 1, 2022

Uh oh!

elizabethandrews commented Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elizabethandrews commented Sep 1, 2022

Uh oh!

steffenlarsen commented Sep 1, 2022

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews commented Sep 6, 2022

Uh oh!

steffenlarsen commented Sep 6, 2022

Uh oh!

Uh oh!

elizabethandrews commented Aug 30, 2022 •

edited

Loading

elizabethandrews commented Sep 1, 2022 •

edited

Loading