[SYCL] Replace hardcoded namespaces with attribute #6674
Conversation
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 <elizabeth.andrews@intel.com>
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
Fznamznon
left a comment
Fznamznon
left a comment
There was a problem hiding this comment.
Just a couple of nits to the tests, otherwise LGTM.
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
premanandrao
left a comment
premanandrao
left a comment
There was a problem hiding this comment.
Just nits, except in one place. Otherwise, looks good.
Signed-off-by: Elizabeth Andrews <elizabeth.andrews@intel.com>
steffenlarsen
left a comment
steffenlarsen
left a comment
There was a problem hiding this comment.
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. |
|
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
left a comment
bader
left a comment
There was a problem hiding this comment.
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.
|
@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 |
|
Failing OCL x64 was a known issue and has since been fixed. CUDA failure does indeed look to be infrastructure-related. |
8 participants
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