[SYCL] Fix SYCL internal enumerators conflict with user defined macro by bso-intel · Pull Request #1188 · intel/llvm

bso-intel · 2020-02-26T02:11:29Z

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

[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 <byoungro.so@intel.com>

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bso-intel · 2020-02-26T02:26:04Z

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

AlexeySachkov · 2020-02-26T08:07:26Z

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

sycl/test/regression/macro_conflict.cpp

bader · 2020-02-26T10:04:16Z

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

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

bso-intel · 2020-02-26T16:10:42Z

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)

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bso-intel · 2020-02-26T16:29:53Z

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

Signed-off-by: Byoungro So <byoungro.so@intel.com>

AlexeySachkov

LGTM

sycl/test/regression/macro_conflict.cpp

sycl/source/detail/scheduler/commands.hpp

sycl/test/regression/macro_conflict.cpp

Signed-off-by : Byoungro So <byoungro.so@intel.com> Co-Authored-By: Alexey Bader <alexey.bader@intel.com>

Signed-off-by: Byoungro So <byoungro.so@intel.com>

Signed-off-by: Byoungro So <byoungro.so@intel.com> Co-Authored-By: Alexey Bader <alexey.bader@intel.com>

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bso-intel · 2020-02-27T00:21:02Z

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

keryell · 2020-02-27T15:56:27Z

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

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bader

LGTM. Thanks.

keryell

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...

sycl/test/regression/macro_conflict.cpp

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bader · 2020-03-02T10:04:12Z

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

keryell

This looks good!

…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) ...

…_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 ...

bso-intel added 4 commits February 24, 2020 22:18

[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 <byoungro.so@intel.com>

Merge remote-tracking branch 'upstream/sycl' into sycl

da3f972

[SYCL] added a regression test for macro conflict

4862f8b

Signed-off-by: Byoungro So <byoungro.so@intel.com>

fixed the clang format check issue

11ad1ed

Signed-off-by: Byoungro So <byoungro.so@intel.com>

AlexeySachkov reviewed Feb 26, 2020

View reviewed changes

romanovvlad assigned AlexeySachkov Feb 26, 2020

fixed regression test compiler option

2a61c01

Signed-off-by: Byoungro So <byoungro.so@intel.com>

fixed a typo

6dec0f2

Signed-off-by: Byoungro So <byoungro.so@intel.com>

AlexeySachkov previously approved these changes Feb 26, 2020

View reviewed changes

bader requested changes Feb 26, 2020

View reviewed changes

bso-intel dismissed AlexeySachkov’s stale review via 7c2e853 February 26, 2020 22:39

bso-intel and others added 5 commits February 26, 2020 16:13

Update sycl/source/detail/scheduler/commands.hpp

4fc8330

Signed-off-by : Byoungro So <byoungro.so@intel.com> Co-Authored-By: Alexey Bader <alexey.bader@intel.com>

fixed clang-format error

8414e8b

Signed-off-by: Byoungro So <byoungro.so@intel.com>

second attempt to fix the format error

c93c0f8

Signed-off-by: Byoungro So <byoungro.so@intel.com>

Update sycl/test/regression/macro_conflict.cpp

85d75cf

Signed-off-by: Byoungro So <byoungro.so@intel.com> Co-Authored-By: Alexey Bader <alexey.bader@intel.com>

changed the enum variable per Alexey's request

50d283b

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bso-intel requested a review from bader February 27, 2020 00:19

removed main() to avoid runtime check per reviewer's request

45d6f99

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bader previously approved these changes Feb 28, 2020

View reviewed changes

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 suggested changes Feb 28, 2020

View reviewed changes

sycl/test/regression/macro_conflict.cpp Outdated Show resolved Hide resolved

bader assigned romanovvlad and unassigned AlexeySachkov Feb 28, 2020

fixed a typo.

5eda190

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bso-intel dismissed bader’s stale review via 5eda190 February 28, 2020 20:26

bso-intel added 7 commits February 28, 2020 16:39

changed enum variables to conform to guideline

65bccf4

Signed-off-by: Byoungro So <byoungro.so@intel.com>

fixed the clang format issue

043f1b8

Signed-off-by: Byoungro So <byoungro.so@intel.com>

fix clang format issue

8bbcbd0

Signed-off-by: Byoungro So <byoungro.so@intel.com>

Merge remote-tracking branch 'upstream/sycl' into sycl

a37642c

fixed LIT test failure

7ad8d19

Signed-off-by: Byoungro So <byoungro.so@intel.com>

fixed the clang format issue

f73a2d9

Signed-off-by: Byoungro So <byoungro.so@intel.com>

fixed format

c86049c

Signed-off-by: Byoungro So <byoungro.so@intel.com>

bader approved these changes Feb 29, 2020

View reviewed changes

keryell approved these changes Mar 2, 2020

View reviewed changes

bader merged commit 0f7e361 into intel:sycl Mar 3, 2020

Conversation

bso-intel commented Feb 26, 2020

Uh oh!

bso-intel commented Feb 26, 2020

Uh oh!

AlexeySachkov commented Feb 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bader commented Feb 26, 2020

Uh oh!

bso-intel commented Feb 26, 2020

Uh oh!

bso-intel commented Feb 26, 2020

Uh oh!

AlexeySachkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bso-intel commented Feb 27, 2020

Uh oh!

keryell commented Feb 27, 2020

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bader commented Mar 2, 2020 •

edited

Loading