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

Fix DAAL examples build failures with C++20 #3072

Merged
merged 5 commits into from
Feb 17, 2025

Conversation

Vika-F
Copy link
Contributor

@Vika-F Vika-F commented Feb 14, 2025

Remove templates ids in constructors to fix error: template-id not allowed for constructor in C++20.


PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.

You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).

Checklist to comply with before moving PR from draft:

PR completeness and readability

  • I have reviewed my changes thoroughly before submitting this pull request.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have added a respective label(s) to PR if I have a permission for that.
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.

Performance

Not applicable.

@Vika-F Vika-F added the bug label Feb 14, 2025
@david-cortes-intel
Copy link
Contributor

@Vika-F If I understand it correctly, the logic for checking that it works is that all examples compile successfully with C++20.

But do examples cover all of the public headers that are offered by oneDAL? Could there perhaps be other files out there that need modifications? Would oneDAL compile under C++20 if you try it? (might also require modifications in internal headers, but could give some pointers as to whether the public headers have sufficient modifications or not).

Also, I think in addition to the C++20 flag, for testing purposes you'd also have to add the flag -Werror=template-id-cdtor.

@Vika-F
Copy link
Contributor Author

Vika-F commented Feb 14, 2025

@david-cortes-intel

  1. I do not think we need to do oneDAL build with C++20 to check that the fix works. Because DAAL examples include "daal.h" which includes all the DAAL headers; and oneDAL examples include "dal.hpp" respectively.
  2. Regarding -Werror=template-id-cdtor. Our examples are compiled with -Wextra and warnings as errors, so no need in adding this option. Previous run of Azure pipelines failed with those template related errors.

@david-cortes-intel
Copy link
Contributor

Curious that it results in errors when trying to deduce templates with GCC:

/home/vsts/work/1/s/__release_lnx_clang/daal/latest/include/oneapi/dal/detail/memory.hpp:135:9: error: no matching function for call to 'deallocate'
  135 |         oneapi::dal::preview::detail::deallocate(alloc_, data, count_);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib/gcc-cross/aarch64-linux-gnu/11/../../../../aarch64-linux-gnu/include/c++/11/bits/shared_ptr_base.h:637:8: note: in instantiation of function template specialization 'oneapi::dal::preview::detail::destroy_delete<std::pair<int, int>, std::allocator<std::pair<int, int>>>::operator()<std::pair<int, int>, true>' requested here
  637 |               __d(__p); // Call _Deleter on __p.
      |               ^
/usr/bin/../lib/gcc-cross/aarch64-linux-gnu/11/../../../../aarch64-linux-gnu/include/c++/11/bits/shared_ptr_base.h:618:4: note: in instantiation of function template specialization 'std::__shared_count<>::__shared_count<std::pair<int, int> *, oneapi::dal::preview::detail::destroy_delete<std::pair<int, int>, std::allocator<std::pair<int, int>>>, std::allocator<void>, void>' requested here
  618 |         : __shared_count(__p, std::move(__d), allocator<void>())
      |           ^
/usr/bin/../lib/gcc-cross/aarch64-linux-gnu/11/../../../../aarch64-linux-gnu/include/c++/11/bits/shared_ptr_base.h:1108:17: note: in instantiation of function template specialization 'std::__shared_count<>::__shared_count<std::pair<int, int> *, oneapi::dal::preview::detail::destroy_delete<std::pair<int, int>, std::allocator<std::pair<int, int>>>, void>' requested here
 1108 |         : _M_ptr(__p), _M_refcount(__p, std::move(__d))
      |                        ^
/usr/bin/../lib/gcc-cross/aarch64-linux-gnu/11/../../../../aarch64-linux-gnu/include/c++/11/bits/shared_ptr.h:178:11: note: in instantiation of function template specialization 'std::__shared_ptr<std::pair<int, int>>::__shared_ptr<std::pair<int, int>, oneapi::dal::preview::detail::destroy_delete<std::pair<int, int>, std::allocator<std::pair<int, int>>>, void>' requested here
  178 |         : __shared_ptr<_Tp>(__p, std::move(__d)) { }
      |           ^
/home/vsts/work/1/s/__release_lnx_clang/daal/latest/include/oneapi/dal/detail/array_impl.hpp:256:23: note: in instantiation of function template specialization 'std::shared_ptr<std::pair<int, int>>::shared_ptr<std::pair<int, int>, oneapi::dal::preview::detail::destroy_delete<std::pair<int, int>, std::allocator<std::pair<int, int>>>, void>' requested here
  256 |         data_owned_ = shared(data, std::forward<Deleter>(deleter));
      |                       ^
/home/vsts/work/1/s/__release_lnx_clang/daal/latest/include/oneapi/dal/array.hpp:534:16: note: (skipping 2 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
  534 |         impl_->reset(detail::default_host_policy{}, data, count, std::forward<Deleter>(deleter));
      |                ^

@Vika-F
Copy link
Contributor Author

Vika-F commented Feb 14, 2025

@david-cortes-intel
Currently I have no idea how this relates to my changes.
Planning to reproduce locally and hopefully fix.

@Vika-F
Copy link
Contributor Author

Vika-F commented Feb 14, 2025

/intelci: run

@ethanglaser
Copy link
Contributor

Do we do any regular validation on C++20 or how was this discovered?

@Vika-F
Copy link
Contributor Author

Vika-F commented Feb 14, 2025

@ethanglaser It was discovered during oneAPI release validation. The examples were built with new GCC that enables C++20 by default.

@Vika-F
Copy link
Contributor Author

Vika-F commented Feb 14, 2025

/intelci: run

@Vika-F Vika-F marked this pull request as ready for review February 14, 2025 20:59
@Vika-F Vika-F merged commit db5c08e into uxlfoundation:main Feb 17, 2025
17 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants