Skip to content

[SYCL] Fix SYCL internal enumerators conflict with user defined macro #1188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 20 commits into from
Mar 3, 2020
Merged

[SYCL] Fix SYCL internal enumerators conflict with user defined macro #1188

merged 20 commits into from
Mar 3, 2020

Conversation

bso-intel
Copy link
Contributor

rebased from upstream to resolve conflict.
added a regression test macro_conflict.cpp

@bso-intel
[SYCL] fix the enum variable conflict with user define
809af69 
[CORC-7433]

Enum variables were too commonly used by users.
This kind of conflicts cannot be avoided 100%, but
we can minimize the chance by using the prefix SYCL_

Signed-off-by: Byoungro So <[email protected]>
@bso-intel
Merge remote-tracking branch 'upstream/sycl' into sycl
da3f972
@bso-intel
[SYCL] added a regression test for macro conflict
4862f8b 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel
fixed the clang format check issue
11ad1ed 
Signed-off-by: Byoungro So <[email protected]>
@bso-intel
Copy link
Contributor Author

@romanovvlad @AlexeySachkov @bader
Please review.
Somehow I cannot see the suggested reviewers in this pull request.

@AlexeySachkov
Copy link
Contributor

BTW, @bader, I wonder why clang-format-check was launched twice, any ideas?

AlexeySachkov
AlexeySachkov reviewed Feb 26, 2020
View reviewed changes
@bader
Copy link
Contributor

bader commented Feb 26, 2020

I wonder why clang-format-check was launched twice, any ideas?

I don't know. Maybe it's triggered manually...

@bso-intel
Copy link
Contributor Author

I wonder why clang-format-check was launched twice, any ideas?

I don't know. Maybe it's triggered manually...

I did not run anything manually.
Somehow the second run was launched and took a long time.
Also, the test "buildbot/sycl-ubu-x64-pr" is waiting for status to be reported very long (still not done)

@bso-intel
Copy link
Contributor Author

@AlexeySachkov , I updated the regression test as you suggested.

@bso-intel
fixed a typo
6dec0f2 
Signed-off-by: Byoungro So <[email protected]>
AlexeySachkov
AlexeySachkov previously approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@AlexeySachkov AlexeySachkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bader
bader requested changes Feb 26, 2020
View reviewed changes
bso-intel and others added 5 commits February 26, 2020 16:13
@bso-intel bso-intel requested a review from bader February 27, 2020 00:19
@bso-intel
Copy link
Contributor Author

@bader , I fixed all your request. Please review.
Thanks.

@keryell
Copy link
Contributor

keryell commented Feb 27, 2020

You could also used lowercase in the enum as bike-shedding attempt

bader
bader previously approved these changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@bader bader changed the title [SYCL] fix the enum variable conflict with user defined macro [SYCL] Fix SYCL internal enumerators conflict with user defined macro Feb 28, 2020
keryell
keryell suggested changes Feb 28, 2020
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would have followed https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly at the first time and there would not have had this issue... :-)
Anyway, macros are evil and fortunately the more modern C++ we use, the less macros we need...

@bader bader assigned romanovvlad and unassigned AlexeySachkov Feb 28, 2020
@bso-intel
fixed a typo.
5eda190 
Signed-off-by: Byoungro So <[email protected]>
@bader
Copy link
Contributor

bader commented Mar 2, 2020 •
edited
Loading

@keryell, please, take a look at the updated version.

keryell
keryell approved these changes Mar 2, 2020
View reviewed changes
Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

@bader bader merged commit 0f7e361 into intel:sycl Mar 3, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 4, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/refa…
3749455 
…ctor_tests

* origin/sycl: (32 commits)
  [SYCL] Fix circular reference between events and queues (intel#1226)
  [CI][Doc] Use SSH to deploy GitHub Pages (intel#1232)
  [SYCL][CUDA][Test] Testing for use of CUDA primary context (intel#1174)
  [SYCL] allow underscore symbol in temporary directory name
  [SYCL] Reject zero length arrays (intel#1153)
  [SYCL] Fix static code analyzis concerns (intel#1189)
  [SYCL] Add more details about the -fintelfpga option (intel#1218)
  [SYCL][CUDA] Select only NVPTX64 device binaries (intel#1223)
  [SYCL] Reverse max work-group size order (intel#1177)
  [SYCL][Doc] Add GroupAlgorithms extension (intel#1079)
  [SYCL] Fix SYCL internal enumerators conflict with user defined macro (intel#1188)
  [SYCL][CUDA] Fixes context release and unnamed context scope (intel#1207)
  [SYCL][CUDA] Fix context creation property parsing
  [CUDA][PI] clang-format pi.h
  [SYCL][CUDA] Handle the case of not having any CUDA device (intel#1212)
  [SYCL] Fix check-sycl-deploy target problems (intel#1165)
  [SYCL] Disable tests which take more than 5 minutes (intel#1220)
  [SYCL] Make context constructors explicit to avoid unintended conversions (intel#1219)
  [SYCL][NFC] Add clang-format configuration file for SYCL LIT tests (intel#1224)
  [SYCL] Fix command cleanup invoked from multiple threads (intel#1214)
  ...
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 5, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/host…
71cbcf6 
…_accessor_refactor

* origin/sycl: (38 commits)
  [SYCL] Fix device::get_devices() with a non-host device type (intel#1235)
  [SYCL][PI][CUDA] Implement kernel and kernel-group information queries (intel#1180)
  [SYCL] Remove default error code value in exception (intel#1150)
  [SYCL] Fix devicelib assert LIT test (intel#1245)
  [SYCL] Set aux-target-cpu for SYCL offload device compilation (intel#1225)
  [SYCL] Remove fabs and ceil from the list of unsupported math functions (intel#1217)
  [SYCL] Fix circular reference between events and queues (intel#1226)
  [CI][Doc] Use SSH to deploy GitHub Pages (intel#1232)
  [SYCL][CUDA][Test] Testing for use of CUDA primary context (intel#1174)
  [SYCL] allow underscore symbol in temporary directory name
  [SYCL] Reject zero length arrays (intel#1153)
  [SYCL] Fix static code analyzis concerns (intel#1189)
  [SYCL] Add more details about the -fintelfpga option (intel#1218)
  [SYCL][CUDA] Select only NVPTX64 device binaries (intel#1223)
  [SYCL] Reverse max work-group size order (intel#1177)
  [SYCL][Doc] Add GroupAlgorithms extension (intel#1079)
  [SYCL] Fix SYCL internal enumerators conflict with user defined macro (intel#1188)
  [SYCL][CUDA] Fixes context release and unnamed context scope (intel#1207)
  [SYCL][CUDA] Fix context creation property parsing
  [CUDA][PI] clang-format pi.h
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keryell keryell keryell approved these changes

@bader bader bader approved these changes

@AlexeySachkov AlexeySachkov AlexeySachkov left review comments

Assignees

@romanovvlad romanovvlad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bso-intel @AlexeySachkov @bader @keryell @romanovvlad