Skip to content

[SYCL]Resolve min/max conflict #1339

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 13 commits into from
Apr 2, 2020
Merged

[SYCL]Resolve min/max conflict #1339

merged 13 commits into from
Apr 2, 2020

Conversation

MochalovaAn
Copy link
Contributor

Adding #undef macros to resolve conflict between definitions of the min
and max function in the CL/sycl.h and windows.h or another min and max
define.

Signed-off-by: Mochalova [email protected]

@MochalovaAn MochalovaAn mentioned this pull request Mar 17, 2020
Merged
@MochalovaAn MochalovaAn changed the title [Xmain]Resole min/max conflict [Xmain]Resolve min/max conflict Mar 17, 2020
@AlexeySachkov AlexeySachkov changed the title [Xmain]Resolve min/max conflict [SYCL] Resolve min/max conflict Mar 17, 2020
erichkeane
erichkeane reviewed Mar 17, 2020
View reviewed changes
Copy link
Contributor

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

The canonical way to do this is to require users of the windows.h header to #define NOMINMAX. That is the way the standard library handles it on windows (and I believe the IDE does by default in many cases).

@MochalovaAn MochalovaAn changed the title [SYCL] Resolve min/max conflict [SYCL]Resolve min/max conflict Mar 18, 2020
@romanovvlad
Copy link
Contributor

romanovvlad commented Mar 18, 2020 •
edited
Loading

Adding braces around min and max function declaration inside the SYCL doesn't help?:
void min(T Val1, T Val2); -> void (min)(T Val1, T Val2)

romanovvlad
romanovvlad requested changes Mar 22, 2020
View reviewed changes
Copy link
Contributor

@romanovvlad romanovvlad left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please add a test and fix clang format concerns.

@MochalovaAn
Copy link
Contributor Author

The order of the macros (first "windows.h" then "CL/sycl.hpp" ) is necessary for proper testing

@bader bader requested a review from romanovvlad March 25, 2020 11:10
@bader
Copy link
Contributor

bader commented Mar 25, 2020

@MochalovaAn, please, fix code formatting issues by applying clang-format to the patch.

@MochalovaAn
Copy link
Contributor Author

@MochalovaAn, please, fix code formatting issues by applying clang-format to the patch.

The order of the macros (first "windows.h" then "CL/sycl.hpp" ) is necessary for proper testing

@bader
Copy link
Contributor

bader commented Mar 31, 2020

@MochalovaAn, please, take a look at LIT test failure on Windows platform.

MochalovaAn and others added 10 commits March 31, 2020 21:33
@MochalovaAn
[SYCL] Resolve min/max conflict
4c85b02 
Adding #undef macros to resolve conflict between definitions of the min
and max function in the CL/sycl.h and windows.h or another min and max
define.
confict type from sycl and windows.h

Signed-off-by: Mochalova <[email protected]>
@MochalovaAn
Apply comments
ebbd5af 
Use adding brackets around min & max function declarations instead of
macros

Signed-off-by: Mochalova <[email protected]>
@MochalovaAn
Clang-format fix
0784185 
Signed-off-by: amochalo <[email protected]>
@MochalovaAn
Clang-format fix
c9b1695 
Signed-off-by: amochalo <[email protected]>
@MochalovaAn
Add test
1218f68 
Signed-off-by: Mochalova <[email protected]>
@MochalovaAn
Clang-format fix for test
0d48a6b 
Signed-off-by: amochalo <[email protected]>
@MochalovaAn
Apply comments
3010f93 
Signed-off-by: Mochalova <[email protected]>
@MochalovaAn
Apply comment
cd9995e 
Signed-off-by: Mochalova <[email protected]>
@MochalovaAn @bader
Update sycl/test/basic_tests/min_max_test.cpp
1fd6365 
Signed-off-by: Mochalova <[email protected]>
Co-Authored-By: Alexey Bader <[email protected]>
@MochalovaAn
Apply suggestion
715a22e 
Signed-off-by: Mochalova <[email protected]>
@MochalovaAn
Apply comment
8089ee0 
Signed-off-by: Mochalova <[email protected]>
@MochalovaAn
Apply comment
38be324 
Signed-off-by: Mochalova <[email protected]>
@MochalovaAn
Apply comment
2eba981 
Signed-off-by: Mochalova <[email protected]>
@bader
Copy link
Contributor

bader commented Apr 1, 2020

@romanovvlad, please, review.

@bader bader merged commit e7d4537 into sycl Apr 2, 2020
@bader bader deleted the MinMaxConflict branch April 2, 2020 11:22
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 6, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/hide…
b5c2289 
…_private_api

* origin/sycl: (614 commits)
  [SYCL][Doc] Update prerequisites in GetStartedGuide (intel#1466)
  [SYCL][USM] Remove vestigial dead code (intel#1474)
  [SYCL-PTX] Fix __spirv_GroupAsyncCopy stride computation (intel#1451)
  [Driver][SYCL] Emit an error if c compilation is forced (intel#1438)
  [SYCL] Fix sycl-post-link when no split and symbols are requested. (intel#1454)
  [SYCL] Change priority of devices in default_selector (intel#1264)
  [CI] Update CODEOWNERS matching rules order (intel#1468)
  [SYCL] Share PFWG lambda object through shared memory (intel#1455)
  [CI] Fix CODEOWNERS file syntax (intel#1464)
  [SYCL][CUDA] Fix active context when creating base event (intel#1447)
  [SYCL] Diagnose implicit declaration of kernel function type (intel#1450)
  [BuildBot] Modify configure script (intel#1421)
  [SYCL] Resolve min/max conflict (intel#1339)
  [CI][BuildBot] Fix configure parameter to turn on/off assertions (intel#1449)
  [SYCL] XFAIL LIT test due to duplicate diagnostic
  [SYCL] Remove explicit sycl_device attribute requirement
  Apply more suggestions
  Apply suggestions
  Translate new set of Intel FPGA Loop Controls
  Translate Intel FPGA force_pow2_depth memory attribute
  ...
@againull againull mentioned this pull request Sep 22, 2022
Merged
AlexeySachkov pushed a commit that referenced this pull request Sep 23, 2022
@againull
[SYCL] Resolve min/max conflict (#6861)
ce11646 
Resolve min/max conflict in the same way as it was done here: #1339

Needed to fix this failure:
https://github.com/intel/llvm/actions/runs/3107743966/jobs/5036187282
@againull againull mentioned this pull request Dec 13, 2022
Merged
againull added a commit that referenced this pull request Dec 13, 2022
@againull
[SYCL][NFC] Fix min/max conflict on Windows (#7768)
0299ac1 
Fix conflict with min/max macro on Windows in a similar way as in the
#1339
This is needed to fix post-commit failure -
https://github.com/intel/llvm/actions/runs/3687931956/jobs/6242122359
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@AlexeySachkov AlexeySachkov AlexeySachkov left review comments

@erichkeane erichkeane erichkeane left review comments

@Fznamznon Fznamznon Fznamznon left review comments

@romanovvlad romanovvlad romanovvlad approved these changes

Assignees

@romanovvlad romanovvlad

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MochalovaAn @romanovvlad @bader @AlexeySachkov @erichkeane @Fznamznon