[SYCL] Import/preprocess boost/mp11 at build time to use in SYCL headers. #5791

kbobrovs · 2022-03-11T19:49:29Z

Preprocessing adapts boost/mp11 headers for use in SYCL headers in a way
that does not conflict with potential use of boost in user code.
Particularly, BOOST_* macros are replaced with SYCL_BOOST_*, APIs
are moved into the top-level sycl::detail namespace. For example,
sycl::detail::boost::mp11::mp_list.

Signed-off-by: Konstantin S Bobrovsky [email protected]

…ers. Preprocessing adapts boost/mp11 headers for use in SYCL headers in a way that does not conflict with potential use of boost in user code. Particularly, `BOOST_*` macros are replaced with `SYCL_BOOST_*`, APIs are moved into the top-level `sycl` namespace. For example, `sycl::boost::mp11::mp_list`. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

gmlueck · 2022-03-11T21:02:44Z

One thing that needs updating for sure is the sycl::boost namespace. Please suggest variants.

We should not pollute the ::sycl namespace. Implementation details go in ::sycl::detail, so maybe ::sycl::detail::boost.

kbobrovs · 2022-03-12T00:18:19Z

One thing that needs updating for sure is the sycl::boost namespace. Please suggest variants.

We should not pollute the ::sycl namespace. Implementation details go in ::sycl::detail, so maybe ::sycl::detail::boost.

Right, we should not. @rolandschulz also suggested ::sycl::detail::boost - I will make the change.

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

- Avoid FetchContent_MakeAvailable, as we just want to download sources, and not to import mp11's cmake files into build. - Fix dependencies to make sure mp11 preprocessed headers are produced when making higher-level targets Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

pvchupin

@alexbatashev, can you also review this please? I'm sure you'll have some good suggestions.

pvchupin · 2022-03-16T18:59:54Z

sycl/CMakeLists.txt

@@ -126,6 +126,68 @@ install(DIRECTORY ${OpenCL_INCLUDE_DIR}/CL
        DESTINATION ${SYCL_INCLUDE_DIR}/sycl
        COMPONENT OpenCL-Headers)

+# -------------- boost/mp11 headers import and preprocessing


sycl/CMakeLists.txt keeps growing, I would suggest to isolate external projects elsewhere. Maybe we should do something similar to opencl deps, basically have mp11 directory on top level with CMakeLists.txt alone and have add_llvm_external_project(mp11) here (see line 112)?

Good point. I refactored the code. But I can't really add it as llvm project, as we talked, so I put mp11 modules into sycl/cmake/modules, leaving only include(<main_mp11_module_name>) in the sycl CMakeLists.txt.

also I fixed errors arising with cmake 3.16 used in CI

sycl/test/basic_tests/boost_mp11_import_sanity_check.cpp

…ake 3.16. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

keryell

Nice, we can use Boost in the implementation? :-)
Just curious, why do you need MP11?

keryell · 2022-03-16T23:37:32Z

sycl/cmake/modules/PreprocessBoostMp11Headers.cmake

+    OUT ${OUT}
+    SRC_PATH ${SRC_PATH}
+    SRC_ID ${SRC_ID}
+)


thanks, fixed

kbobrovs · 2022-03-17T00:39:36Z

Nice, we can use Boost in the implementation? :-)
Just curious, why do you need MP11?

Sort of. We can only boost::mp11 in our implementation for now :)
This is needed for invoke_simd and property_lists @steffenlarsen works on.

kbobrovs · 2022-03-17T02:29:08Z

@pvchupin, @keryell, @gmlueck - please review, comments addressed.

@intel/llvm-reviewers-runtime, ping

pvchupin · 2022-03-17T17:29:03Z

ping @alexbatashev

gmlueck

The header file path <sycl/detail/boost/mp11.hpp> and the namespace sycl::detail::boost::mp11 both look OK tome.

v-klochkov

Looks good to me.
I advice to ask for one more reviewer to look at these CMake change.

sycl/cmake/modules/PreprocessBoostMp11Headers.cmake

Co-authored-by: Alexey Bader <[email protected]>

kbobrovs · 2022-03-18T16:09:52Z

Fixed comment according to review comments in [7cacacb].
@bader, @alexbatashev, @v-klochkov, @gmlueck, @pvchupin - please re-approve.

bader

Fixed comment according to review comments in [7cacacb]. @bader, @alexbatashev, @v-klochkov, @gmlueck, @pvchupin - please re-approve.

I think this typo fix won't change the status and we can consider this PR approved by all these folks. Are you still waiting for reviews from @sergey-semenov, @steffenlarsen and @rolandschulz?

kbobrovs · 2022-03-18T17:19:26Z

I think this typo fix won't change the status and we can consider this PR approved by all these folks. Are you still waiting for reviews from @sergey-semenov, @steffenlarsen and @rolandschulz?

No, I think I have enough reviews at this point.

kbobrovs · 2022-03-19T00:52:32Z

@steffenlarsen, FYI - you can now use mp11

Currently, compile-time properties are sorted using a hand-written implementation of merge-sort. However, this PR propose to sort compile-time properties using Boost's MP11 metaprogramming library instead. Mp11's sorting functionality is more optimized and it also improves the overall code readability (83 LOC vs just 11 LOC). PS:- This PR does not introduce any new dependency as Mp11 is already integrated into the code base (See [#5791](#5791)).

This PR reintroduces #8168 that was reverted by #8325 due to some build errors with MSVC compilers. Upon inspection, it was found that the build errors occur only in the case of incremental builds. With clean builds - i.e. completely removing the build directory and re-configuring/re-building again - there won't be any errors. -------------- Currently, compile-time properties are sorted using a hand-written implementation of merge-sort. However, this PR propose to sort compile-time properties using Boost's MP11 metaprogramming library instead. Mp11's sorting functionality is more optimized and it also improves the overall code readability (83 LOC vs just 11 LOC). PS:- This PR does not introduce any new dependency as Mp11 is already integrated into the code base (See #5791).

kbobrovs requested a review from a team as a code owner March 11, 2022 19:49

kbobrovs requested review from sergey-semenov, pvchupin, rolandschulz, alexbatashev, steffenlarsen, bader and gmlueck March 11, 2022 19:49

kbobrovs added 4 commits March 11, 2022 20:59

Fix clang-format.

81fe066

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Change sycl::boost namespace to sycl::detail::boost.

8793088

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Fix test - return value from main().

6166e5e

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

Add oroginal boost test location

e0c97c6

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

kbobrovs force-pushed the import_boost_mp11 branch from 1124ea5 to e0c97c6 Compare March 12, 2022 05:24

Fix copyright

197019c

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

rolandschulz previously approved these changes Mar 14, 2022

View reviewed changes

kbobrovs dismissed rolandschulz’s stale review via 49efd65 March 15, 2022 01:48

Log boost/mp11 download message before actual download

8b063ac

Signed-off-by: Konstantin S Bobrovsky <[email protected]>

pvchupin reviewed Mar 16, 2022

View reviewed changes

Factor out mp11 cmake code into a separate file, make it work with cm…

1390474

…ake 3.16. Signed-off-by: Konstantin S Bobrovsky <[email protected]>

kbobrovs requested review from pvchupin and rolandschulz March 16, 2022 23:19

keryell reviewed Mar 16, 2022

View reviewed changes

kbobrovs added 2 commits March 16, 2022 17:42

Add EOL to PreprocessBoostMp11Headers.cmake

ff0bc41

Removed redundant trailing EOL from AddBoostMp11Headers.cmake

0ce48e5

bader dismissed stale reviews from keryell and pvchupin via 90d1dbf March 17, 2022 10:44

kbobrovs requested a review from pvchupin March 17, 2022 15:54

pvchupin previously approved these changes Mar 17, 2022

View reviewed changes

gmlueck previously approved these changes Mar 17, 2022

View reviewed changes

v-klochkov previously approved these changes Mar 17, 2022

View reviewed changes

alexbatashev previously approved these changes Mar 18, 2022

View reviewed changes

bader previously approved these changes Mar 18, 2022

View reviewed changes

sycl/cmake/modules/PreprocessBoostMp11Headers.cmake Outdated Show resolved Hide resolved

Update sycl/cmake/modules/PreprocessBoostMp11Headers.cmake

7cacacb

Co-authored-by: Alexey Bader <[email protected]>

kbobrovs dismissed stale reviews from bader, alexbatashev, v-klochkov, gmlueck, and pvchupin via 7cacacb March 18, 2022 16:06

bader approved these changes Mar 18, 2022

View reviewed changes

gmlueck approved these changes Mar 18, 2022

View reviewed changes

pvchupin approved these changes Mar 18, 2022

View reviewed changes

alexbatashev approved these changes Mar 18, 2022

View reviewed changes

kbobrovs merged commit cae91b2 into intel:sycl Mar 19, 2022

kbobrovs deleted the import_boost_mp11 branch May 4, 2022 17:47

kbobrovs mentioned this pull request May 25, 2022

[ESIMD] Add set_kernel_properties API and use_double_grf property. #6182

Merged

bader mentioned this pull request May 26, 2022

Compilation with -DVL=... crashes in type_list.hpp #2981

Open

uditagarwal97 mentioned this pull request Jan 31, 2023

[SYCL] Use mp11 to sort compile-time properties #8168

Merged

uditagarwal97 mentioned this pull request Feb 16, 2023

[SYCL] Re-introduce "Use mp11 to sort compile-time properties" #8381

Merged

[SYCL] Import/preprocess boost/mp11 at build time to use in SYCL headers. #5791

[SYCL] Import/preprocess boost/mp11 at build time to use in SYCL headers. #5791

Uh oh!

Conversation

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

Uh oh!

Uh oh!

gmlueck commented Mar 11, 2022

Uh oh!

kbobrovs commented Mar 12, 2022

Uh oh!

pvchupin left a comment

Choose a reason for hiding this comment

Uh oh!

pvchupin Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

kbobrovs Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

kbobrovs Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keryell left a comment

Choose a reason for hiding this comment

Uh oh!

keryell Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

kbobrovs Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

kbobrovs commented Mar 17, 2022

Uh oh!

kbobrovs commented Mar 17, 2022

Uh oh!

pvchupin commented Mar 17, 2022

Uh oh!

gmlueck left a comment

Choose a reason for hiding this comment

Uh oh!

v-klochkov 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!

kbobrovs commented Mar 18, 2022

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

kbobrovs commented Mar 18, 2022

Uh oh!

kbobrovs commented Mar 19, 2022

Uh oh!

Uh oh!

kbobrovs commented Mar 11, 2022 •

edited

Loading

v-klochkov left a comment •

edited

Loading