[SYCL] Add clang support for device_global #5597

schittir · 2022-02-16T17:57:25Z

This patch implements the basic functionality and some diagnostics
for SYCL device_global attributes. The rest of the diagnostics are to
be added in a subsequent PR.

This patch includes:

Add [[__sycl_detail__::device_global]] attribute and use it to check
some restrictions on variable declarations of device_global type
Add [[__sycl_detail__::global_variable_allowed]] attribute and avoid
diagnosing err_sycl_restrict if variables of device_global type use it
Adds "sycl-unique-id", an LLVM IR attribute, to provide a unique
string identifier for each device global variable

Fznamznon

The functional part (under it I mean the changes described by design doc - https://github.com/intel/llvm/blob/sycl/sycl/doc/design/DeviceGlobal.md, i.e. everything except diagnostics) looks ok to me. Let's focus on that and add some testing:

We need IR checking that sycl-unique-id is properly attached
We need Sema test checking that device_global variable can be captured to SYCL kernel.

clang/include/clang/Basic/Attr.td

clang/include/clang/Basic/AttrDocs.td

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/CodeGen/CodeGenModule.cpp

clang/lib/Sema/SemaExpr.cpp

clang/lib/Sema/SemaDecl.cpp

clang/include/clang/Sema/Sema.h

clang/include/clang/Basic/Attr.td

clang/test/CodeGenSYCL/Inputs/sycl.hpp

- Check type of the variable, not the variable itself when checking presence of the attribute - Fix generation of LLVM IR attribute - Add the CodeGen test

clang/lib/Sema/SemaDecl.cpp

clang/test/CodeGenSYCL/device_global.cpp

clang/lib/Sema/SemaDeclAttr.cpp

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/lib/CodeGen/CodeGenModule.h

clang/test/CodeGenSYCL/Inputs/sycl.hpp

clang/test/SemaSYCL/device_global.cpp

clang/include/clang/Basic/Attr.td

clang/lib/Sema/Sema.cpp

clang/include/clang/Sema/Sema.h

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/include/clang/Sema/Sema.h

clang/test/SemaSYCL/device_global.cpp

clang/include/clang/Basic/AttrDocs.td

smanna12 · 2022-03-03T20:37:21Z

@schittir, please fix the conflicts

schittir · 2022-03-11T18:35:59Z

Please also update PR description to include note about upcoming patch which covers diagnostics.

Done.

clang/lib/CodeGen/CodeGenModule.cpp

schittir · 2022-03-13T22:48:33Z

I am not sure how to reproduce the failing "timed-out" tests.
SYCL :: Plugin/level_zero_batch_test_copy_with_compute.cpp test failed with the latest commit, whereas the last time, SYCL :: ESIMD/kmeans/kmeans.cpp failed. The latter doesn't fail anymore, leading me to think that these are flaky...
I will look into this more, but if anybody has any thoughts or previous experience with this, please feel free to share. Thank you.

smanna12 · 2022-03-13T23:05:26Z

I am not sure how to reproduce the failing "timed-out" tests. SYCL :: Plugin/level_zero_batch_test_copy_with_compute.cpp test failed with the latest commit, whereas the last time, SYCL :: ESIMD/kmeans/kmeans.cpp failed. The latter doesn't fail anymore, leading me to think that these are flaky... I will look into this more, but if anybody has any thoughts or previous experience with this, please feel free to share. Thank you.

@schittir I am seeing same timeout test issue in other PR's (#4533 (comment))
https://github.com/intel/llvm/runs/5518378302?check_suite_focus=true
Timed Out Tests (1):
SYCL :: Plugin/level_zero_batch_test_copy_with_compute.cpp

I am seeing same timeout test issue in other PR's (#5782)
https://github.com/intel/llvm/runs/5518378332?check_suite_focus=true
Timed Out Tests (1):
SYCL :: ESIMD/kmeans/kmeans.cpp

Both failures are unrelated to your patch.

schittir · 2022-03-13T23:28:01Z

I am not sure how to reproduce the failing "timed-out" tests. SYCL :: Plugin/level_zero_batch_test_copy_with_compute.cpp test failed with the latest commit, whereas the last time, SYCL :: ESIMD/kmeans/kmeans.cpp failed. The latter doesn't fail anymore, leading me to think that these are flaky... I will look into this more, but if anybody has any thoughts or previous experience with this, please feel free to share. Thank you.

@schittir I am seeing same timeout test issue in other PR's (#4533 (comment)) https://github.com/intel/llvm/runs/5518378302?check_suite_focus=true Timed Out Tests (1): SYCL :: Plugin/level_zero_batch_test_copy_with_compute.cpp

I am seeing same timeout test issue in other PR's (#5782) https://github.com/intel/llvm/runs/5518378332?check_suite_focus=true Timed Out Tests (1): SYCL :: ESIMD/kmeans/kmeans.cpp

Both failures are unrelated to your patch.

Thank you, Soumi!

clang/lib/CodeGen/CodeGenModule.cpp

smanna12

Changes LGTM.

Fznamznon

Looks ok to me. But I'd like to get approve from @elizabethandrews too.

elizabethandrews

LGTM. Thanks!

clang/include/clang/Basic/AttrDocs.td

clang/test/CodeGenSYCL/device_global.cpp

schittir · 2022-03-15T20:31:19Z

@smanna12 @elizabethandrews could you please take a look again?

schittir · 2022-03-16T00:02:02Z

Test failures unrelated to this PR.
The following timed-out:
SYCL :: KernelAndProgram/multiple-kernel-linking.cpp
SYCL :: Plugin/level_zero_batch_test.cpp
SYCL :: Plugin/level_zero_batch_test_copy_with_compute.cpp
SYCL :: Plugin/retain_events.cpp
SYCL :: Printf/float.cpp
SYCL :: USM/copy.cpp

[SYCL] Add clang support for device_global

0251bfb

Fznamznon reviewed Feb 16, 2022

View reviewed changes

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

schittir added 2 commits February 16, 2022 12:57

Remove unused lines i.e., Merge attribute method calls and SYCLUniqueID

117de59

Address some comments, fix format, remove unused lines

a96d57a

Fznamznon reviewed Feb 17, 2022

View reviewed changes

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

Fznamznon reviewed Feb 17, 2022

View reviewed changes

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

Fznamznon reviewed Feb 17, 2022

View reviewed changes

clang/test/CodeGenSYCL/Inputs/sycl.hpp Outdated Show resolved Hide resolved

schittir and others added 3 commits February 17, 2022 10:18

Add Sema test; address more comments

f2230af

Some fixes in order to pass CodeGen test

0fb176d

- Check type of the variable, not the variable itself when checking presence of the attribute - Fix generation of LLVM IR attribute - Add the CodeGen test

Refactor isSyclGlobalVariableAllowedType

e4c15c4

schittir mentioned this pull request Feb 26, 2022

[SYCL] Emit integration footer and header for device_global variables #5576

Merged

Fix some test cases; address some comments

af8a294

Fznamznon reviewed Feb 28, 2022

View reviewed changes

schittir added 2 commits February 28, 2022 21:46

Remove explicit attribute handling; change diagnostic message; add test

feb841b

Add diagnostic to test

df337a7

Fznamznon reviewed Mar 1, 2022

View reviewed changes

clang/lib/CodeGen/CodeGenModule.h Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/Inputs/sycl.hpp Outdated Show resolved Hide resolved

clang/test/SemaSYCL/device_global.cpp Show resolved Hide resolved

clang/test/SemaSYCL/device_global.cpp Outdated Show resolved Hide resolved

Address Mariya's comments

748e8ce

smanna12 reviewed Mar 1, 2022

View reviewed changes

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

smanna12 reviewed Mar 1, 2022

View reviewed changes

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

schittir and others added 3 commits March 2, 2022 14:35

Fix typo; Refactor methods;

d84428c

Fix quotes, update diag messages, report private members

0343158

Change DeviceGlobalType checking call

6d6d7dd

Fznamznon reviewed Mar 3, 2022

View reviewed changes

clang/include/clang/Sema/Sema.h Outdated Show resolved Hide resolved

Move isSyclGlobalType definition to header

679b5f0

schittir marked this pull request as ready for review March 3, 2022 18:55

schittir requested a review from a team as a code owner March 3, 2022 18:55

Fznamznon reviewed Mar 3, 2022

View reviewed changes

Address latest comments

5ecd545

schittir added 2 commits March 11, 2022 12:52

Add case where device_global attributes are applied to the wrong subject

1c53934

Attribute doesn't apply to this type

e489d50

elizabethandrews reviewed Mar 11, 2022

View reviewed changes

clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved

Add comments in CodeGenModule.cpp

0709448

schittir requested review from Fznamznon, elizabethandrews and smanna12 March 11, 2022 22:50

Fznamznon reviewed Mar 14, 2022

View reviewed changes

clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved

clang/lib/CodeGen/CodeGenModule.cpp Outdated Show resolved Hide resolved

Change comments

d4647f6

smanna12 previously approved these changes Mar 14, 2022

View reviewed changes

schittir requested a review from Fznamznon March 15, 2022 01:41

This was referenced Mar 15, 2022

Add the remaining diagnostics to device_global implementation #5809

Closed

[SYCL] Add the remaining diagnostics to device_global implementation #5810

Merged

Fznamznon reviewed Mar 15, 2022

View reviewed changes

elizabethandrews previously approved these changes Mar 15, 2022

View reviewed changes

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

clang/test/CodeGenSYCL/device_global.cpp Show resolved Hide resolved

Change "customer code" to "user code" in documentation

ef0f5d0

schittir dismissed stale reviews from elizabethandrews and smanna12 via ef0f5d0 March 15, 2022 20:29

schittir requested review from elizabethandrews and smanna12 March 15, 2022 20:30

smanna12 approved these changes Mar 15, 2022

View reviewed changes

elizabethandrews approved these changes Mar 15, 2022

View reviewed changes

bader merged commit c42abb5 into intel:sycl Mar 16, 2022

[SYCL] Add clang support for device_global #5597

[SYCL] Add clang support for device_global #5597

Uh oh!

Conversation

schittir commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smanna12 commented Mar 3, 2022

Uh oh!

schittir commented Mar 11, 2022

Uh oh!

Uh oh!

schittir commented Mar 13, 2022

Uh oh!

smanna12 commented Mar 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schittir commented Mar 13, 2022

Uh oh!

Uh oh!

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

schittir commented Mar 15, 2022

Uh oh!

schittir commented Mar 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

schittir commented Feb 16, 2022 •

edited

Loading

Fznamznon left a comment •

edited

Loading

smanna12 commented Mar 13, 2022 •

edited

Loading