Skip to content
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.

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

[CI] Move testing to Ubuntu 24.04 everywhere #16463

Open
wants to merge 6 commits into
base: sycl
Choose a base branch
from

Conversation

sarnex
Copy link
Contributor

@sarnex sarnex commented Dec 23, 2024

Move testing to Ubuntu 24.04, as 22.04 support is being dropped in various places.

Also, clean up the oneAPI image to not copy-paste the normal build image.

I didn't change the nightly to publish 24.04 images (but it uses them for testing) because users might actually use that and I don't want to break them.

| tee /usr/share/keyrings/oneapi-archive-keyring.gpg > /dev/null && \
echo "deb [signed-by=/usr/share/keyrings/oneapi-archive-keyring.gpg] https://apt.repos.intel.com/oneapi all main" \
| tee /etc/apt/sources.list.d/oneAPI.list

# Install the ROCM kernel driver and oneAPI
RUN apt update && apt install -yqq rocm-dev intel-oneapi-compiler-dpcpp-cpp && \
RUN apt update && apt install -yqq intel-oneapi-compiler-dpcpp-cpp && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Installing DPC++ using apt will install the latest public release of DPC++, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, I could guard the version so it doesn't auto-update but I don't expect new releases to break anything. If they do, I'll add the guard.

Copy link
Contributor

@uditagarwal97 uditagarwal97 Dec 26, 2024

Choose a reason for hiding this comment

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

If they do, I'll add the guard.

I think we should add the version guard proactively, perhaps in a separate PR. This would save us the effort of debugging unrelated failures, if newer DPC++ releases break anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. it's easy enough, I can add it here.

@@ -35,7 +35,7 @@ jobs:
build_cache_root: "/__w/llvm"
build_cache_suffix: default
build_artifact_suffix: default
build_configure_extra_args: --no-assertions --hip --cuda --native_cpu --cmake-opt="-DSYCL_ENABLE_STACK_PRINTING=ON" --cmake-opt="-DSYCL_LIB_WITH_DEBUG_SYMBOL=ON"
build_configure_extra_args: --no-assertions --hip --cuda --native_cpu --cmake-opt="-DSYCL_ENABLE_STACK_PRINTING=ON" --cmake-opt="-DSYCL_LIB_WITH_DEBUG_SYMBOL=ON --cmake-opt=-DCMAKE_CXX_FLAGS="-Wno-mismatched-new-delete"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was -Wno-mismatched-new-delete needed? Can we add a comment here highlighting the need for this flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tried to fix the warning actually but wasn't sure what to do. I think you might have worked in the code the warning is from before so if you have an idea let me know, if not I'll just add a comment. Here's the warning:

In file included from /usr/include/c++/13/bits/std_thread.h:43,
                 from /usr/include/c++/13/thread:45,
                 from /tmp/llvm/build/include/sycl/detail/spinlock.hpp:14,
                 from /tmp/llvm/sycl/source/detail/global_handler.hpp:11,
                 from /tmp/llvm/sycl/source/detail/config.hpp:11,
                 from /tmp/llvm/sycl/source/detail/adapter.hpp:11,
                 from /tmp/llvm/sycl/source/detail/ur_utils.hpp:11,
                 from /tmp/llvm/sycl/source/detail/device_binary_image.hpp:10,
                 from /tmp/llvm/sycl/source/detail/device_binary_image.cpp:9:
In member function 'void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = char]',
    inlined from 'std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = char; _Dp = std::default_delete<char>]' at /usr/include/c++/13/bits/unique_ptr.h:404:17,
    inlined from 'static std::unique_ptr<char> sycl::_V1::detail::ZSTDCompressor::DecompressBlob(const char*, size_t, size_t&)' at /tmp/llvm/sycl/source/detail/compression.hpp:139:3,
    inlined from 'void sycl::_V1::detail::CompressedRTDeviceBinaryImage::Decompress()' at /tmp/llvm/sycl/source/detail/device_binary_image.cpp:258:54:
/usr/include/c++/13/bits/unique_ptr.h:99:9: error: 'void operator delete(void*, std::size_t)' called on pointer returned from a mismatched allocation function [-Werror=mismatched-new-delete]
   99 |         delete __ptr;
      |         ^~~~~~~~~~~~
In file included from /tmp/llvm/sycl/source/detail/device_binary_image.cpp:13:
In static member function 'static std::unique_ptr<char> sycl::_V1::detail::ZSTDCompressor::DecompressBlob(const char*, size_t, size_t&)',
    inlined from 'void sycl::_V1::detail::CompressedRTDeviceBinaryImage::Decompress()' at /tmp/llvm/sycl/source/detail/device_binary_image.cpp:258:54:
/tmp/llvm/sycl/source/detail/compression.hpp:119:66: note: returned from 'void* operator new [](std::size_t)'
  119 |     auto dstBuffer = std::unique_ptr<char>(new char[dstBufferSize]);
      |                                                                  ^
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I know what's going wrong here. I'll open a PR for fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks! Please send it to me once you post it so I can test it, I don't think CI here will replicate the issue because I guess only gcc 13 catches it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would commit changes in SYCL header and SYCL test in a separate PR. They seem to be useful regardless of Ubuntu version used by CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #16480, thx

@bader
Copy link
Contributor

bader commented Dec 26, 2024

I also had to cherry pick an upstream patch that is needed to fix an issue exposed by the newer GCC version in Ubuntu 24.04.

Please, commit this change in a separate PR.

@sarnex
Copy link
Contributor Author

sarnex commented Dec 26, 2024

Please, commit this change in a separate PR.

Done, #16479

@sarnex
Copy link
Contributor Author

sarnex commented Dec 26, 2024

Sorry for force push, got rid of changes that were merged as part of other PRs

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.

If we address new warnings in a separate PR and remove --cmake-opt=-DCMAKE_CXX_FLAGS="-Wno-mismatched-new-delete" from .github/workflows/sycl-post-commit.yml, it will be perfect PR. :)

@bader
Copy link
Contributor

bader commented Dec 26, 2024

Sorry for force push, got rid of changes that were merged as part of other PRs

git merge works too. ;)

@bader
Copy link
Contributor

bader commented Dec 26, 2024

@sarnex, don't forget to update the PR description.

@sarnex
Copy link
Contributor Author

sarnex commented Dec 26, 2024

git merge works too. ;)

Yeah I probably should have done that, for some reason I thought I should drop the cherry-picked commit but merge probably would have done the right thing.

don't forget to update the PR description.

will do thx

Signed-off-by: Sarnie, Nick <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants