Skip to content

[NFC][SYCL] Switch to std::enable_if #7628

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 4 commits into from
Dec 5, 2022
Merged

[NFC][SYCL] Switch to std::enable_if #7628

merged 4 commits into from
Dec 5, 2022

Conversation

aelovikov-intel
Copy link
Contributor

No description provided.

@aelovikov-intel aelovikov-intel requested review from a team as code owners December 2, 2022 22:42
aelovikov-intel
aelovikov-intel commented Dec 2, 2022
View reviewed changes
sycl/include/sycl/detail/stl_type_traits.hpp
@@ -20,9 +20,6 @@ namespace detail {
// Type traits identical to those in std in newer versions. Can be removed when
// SYCL requires a newer version of the C++ standard.
// C++14
template <bool B, class T = void>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sycl::detail::enable_if is removed here.

@bader
Copy link
Contributor

bader commented Dec 2, 2022

No "#include <sycl/include/sycl/detail/stl_type_traits.hpp>" -> "#include <type_traits>"? I suppose this type of transformation is not automated. Right?

@aelovikov-intel
Copy link
Contributor Author

aelovikov-intel commented Dec 2, 2022 •
edited
Loading

No "#include <sycl/include/sycl/detail/stl_type_traits.hpp>" -> "#include <type_traits>"?

I plan to switch different utilities one by one. Once all uses are cleaned up, I'll remove the header.

I suppose this type of transformation is not automated. Right?

I don't understand the question. What I did is

  1. Emacs' M-x rgrep enable_if
  2. Switch to grep-writable-mode, perform regexp, save
  3. Run clang-format

@bader
Copy link
Contributor

bader commented Dec 2, 2022

No "#include <sycl/include/sycl/detail/stl_type_traits.hpp>" -> "#include <type_traits>"?

I plan to switch different utilities one by one. Once all uses are cleaned up, I'll remove the header.

I suppose this type of transformation is not automated. Right?

I don't understand the question. What I did is

  1. Emacs' M-x rgrep enable_if
  2. Switch to grep-writable-mode, perform regexp, save
  3. Run clang-format

Right. I assume that some files are using only enable_if utility. If so, we can include the right header for these files right away. The problem is you need check if any other utility functions are not used. My question was about automating this task with tools like include-what-you-use. If we can't update includes automatically, I don't think it's worth doing manually.

@aelovikov-intel
Copy link
Contributor Author

If we can't update includes automatically, I don't think it's worth doing manually.

I don't have experience with it and setting it up might take more time than removing the header completely :)

kbobrovs
kbobrovs approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

ESIMD LGTM

@againull
Copy link
Contributor

againull commented Dec 3, 2022

Could you please fix RHEL build failure, seems relate: http://icl-jenkins2.sc.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FBuild_PR_RHEL/detail/Build_PR_RHEL/3931/pipeline/152

@pvchupin pvchupin closed this Dec 5, 2022
@pvchupin pvchupin reopened this Dec 5, 2022
pvchupin
pvchupin approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@pvchupin pvchupin left a comment

Choose a reason for hiding this comment

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

cmake changes looks ok

@pvchupin pvchupin merged commit 74833b2 into intel:sycl Dec 5, 2022
@aelovikov-intel aelovikov-intel mentioned this pull request Dec 6, 2022
Merged
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Dec 6, 2022
@aelovikov-intel
[SYCL][CMake][LibDevice] Avoid gcc-toolchain fixup for /bin/g++
2d5adad 
Should fix "/verify with" in llvm-test-suite. The issue was introduced
in intel#7628.
pvchupin pushed a commit that referenced this pull request Dec 6, 2022
@aelovikov-intel
[SYCL][CMake][LibDevice] Avoid gcc-toolchain fixup for /bin/g++ (#7665)
44b98a5 
Should fix "/verify with" in llvm-test-suite. The issue was introduced
in #7628.
@hdelan
Copy link
Contributor

hdelan commented Dec 13, 2022 •
edited
Loading

This PR has introduced a build failure due to missing includes, ping @bader @pvchupin

[148/220] Generating ../../lib/libsycl-fallback-complex.spv
FAILED: lib/libsycl-fallback-complex.spv /home/hugh/llvm/build/lib/libsycl-fallback-complex.spv 
cd /home/hugh/llvm/build/tools/libdevice && /home/hugh/llvm/build/bin/clang-16 -fsycl-device-only -fno-sycl-use-bitcode -Wno-sycl-strict -Wno-undefined-internal -sycl-std=2020 --gcc-toolchain=/usr/lib -fno-sycl-libspirv -fno-bundle-offload-arch -nocudalib /home/hugh/llvm/libdevice/fallback-complex.cpp -o /home/hugh/llvm/build/./lib/libsycl-fallback-complex.spv
clang-16: warning: ignoring '-fno-sycl-libspirv' option as it is not currently supported for target 'spir64-unknown-unknown' [-Woption-ignored]
clang-16: warning: argument unused during compilation: '-fno-bundle-offload-arch' [-Wunused-command-line-argument]
/home/hugh/llvm/libdevice/fallback-complex.cpp:12:10: fatal error: 'cmath' file not found
#include <cmath>
         ^~~~~~~
1 error generated.

hdelan
hdelan reviewed Dec 13, 2022
View reviewed changes
libdevice/cmake/modules/SYCLLibdevice.cmake
@@ -32,6 +32,15 @@ set(compile_opts
-sycl-std=2020
)

if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
Copy link
Contributor

@hdelan hdelan Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What was the reason for this change @aelovikov-intel ? When using ccache, this gcc_install_dir will resolve to /usr/lib/ccache, which means that compilation fails due to missing headers

Copy link
Contributor

Choose a reason for hiding this comment

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

#7761

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our testing on a system with older libc where gcc installation is in non-default location (as default is too old).

@hdelan hdelan mentioned this pull request Dec 13, 2022
Merged
pvchupin pushed a commit that referenced this pull request Dec 13, 2022
@hdelan
[SYCL][libdevice] Add check that ccache is not being used (#7761)
4c203bb 
If ccache was being used, this cmake check would set `gcc_bin_dir` to
`/usr/lib/ccache` and `gcc_install_dir` to `/usr/lib`.

This would cause build failures due to missing headers. See PR
#7628 and comment
#7628 (comment)
Fznamznon added a commit to Fznamznon/llvm that referenced this pull request Dec 14, 2022
@Fznamznon
Revert "[NFC][SYCL] Switch to std::enable_if (intel#7628)"
0feeab9 
This reverts commit 74833b2.
We found issues when trying to build the project on RHEL7 systems that we need
in our OS support matrix.
steffenlarsen pushed a commit that referenced this pull request Dec 14, 2022
@Fznamznon
Revert "[NFC][SYCL] Switch to std::enable_if (#7628)" (#7779)
84693b6 
This reverts commit 74833b2. We found
issues when trying to build the project on RHEL7 systems that we need in
our OS support matrix.
@aelovikov-intel aelovikov-intel deleted the std-enable-if branch April 7, 2023 15:56
@aelovikov-intel aelovikov-intel restored the std-enable-if branch April 7, 2023 15:56
@aelovikov-intel aelovikov-intel mentioned this pull request Apr 21, 2023
Merged
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Apr 21, 2023
@aelovikov-intel
[NFC][SYCL] Switch to std:: equivalents for utilities in stl_type_tra…
8d1f841 
…its.hpp

Re-commits intel#7628 and intel#7668 that were previously reverted due to build
issues on RHEL systems. The issues were fixed by introducing
`SYCL_LIBDEVICE_GCC_TOOLCHAIN` cmake configuration option in intel#7771.
againull pushed a commit that referenced this pull request Apr 24, 2023
@aelovikov-intel
[NFC][SYCL] Switch to std:: equivalents for utilities in stl_type_tra…
1bcc9f0 
…its.hpp (#9162)

Re-commits #7628 and #7668 that were previously reverted due to build
issues on RHEL systems. The issues were fixed by introducing
`SYCL_LIBDEVICE_GCC_TOOLCHAIN` cmake configuration option in #7771.
@aelovikov-intel aelovikov-intel deleted the std-enable-if branch May 1, 2023 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdelan hdelan hdelan left review comments

@kbobrovs kbobrovs kbobrovs approved these changes

@v-klochkov v-klochkov v-klochkov approved these changes

@pvchupin pvchupin pvchupin approved these changes

@bso-intel bso-intel Awaiting requested review from bso-intel bso-intel is a code owner automatically assigned from intel/llvm-reviewers-runtime

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@aelovikov-intel @bader @againull @hdelan @kbobrovs @v-klochkov @pvchupin