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

Remove template parameters from copy constructor #6514

Merged
merged 2 commits into from
Jul 5, 2024

Conversation

hgminh95
Copy link
Contributor

@hgminh95 hgminh95 commented Jul 2, 2024

Contributes to #6033
Contributes to #6522

C++23 no longer allow template parameters in constructor

@hgminh95 hgminh95 marked this pull request as draft July 2, 2024 05:37
@hgminh95
Copy link
Contributor Author

hgminh95 commented Jul 2, 2024

@microsoft-github-policy-service agree

@hgminh95 hgminh95 marked this pull request as ready for review July 2, 2024 05:46
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for this! I've edited the description to link to #6033.

With this change, were you able to compile LightGBM using the C++20 (or even better, C++23 standard)? Or is this just one step towards that?

Either way, we appreciate the help!

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Makes sense to me, based on my read of e.g. https://stackoverflow.com/a/63514123/3986677 and the bits of the C++20 standard linked from it.

Thanks for the help!

@jameslamb jameslamb merged commit a5054f7 into microsoft:master Jul 5, 2024
41 checks passed
@hgminh95
Copy link
Contributor Author

hgminh95 commented Jul 12, 2024

With this change, were you able to compile LightGBM using the C++20 (or even better, C++23 standard)? Or is this just one step towards that?

Can confirm I can compile with C++23, gcc-12. However, it requires me to remove the __mm_malloc macros - #4331 (but this seem be a GCC thing)

@jameslamb
Copy link
Collaborator

Ok interesting, thank you for that! I don't recall the history for these _mm_malloc macros:

#if defined(_MSC_VER)
#include <malloc.h>
#elif MM_MALLOC
#include <mm_malloc.h>
// https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
// https://www.oreilly.com/library/view/mac-os-x/0596003560/ch05s01s02.html
#elif defined(__GNUC__) && defined(HAVE_MALLOC_H)
#include <malloc.h>
#define _mm_malloc(a, b) memalign(b, a)
#define _mm_free(a) free(a)
#else
#include <stdlib.h>
#define _mm_malloc(a, b) malloc(a)
#define _mm_free(a) free(a)
#endif

Maybe @StrikerRUS or @guolinke will remember why they were added in #2699.

@guolinke
Copy link
Collaborator

sometimes we need the memory allocation with aligned address, like

template <class U> CHAllocator(const CHAllocator<U>& other);
T* allocate(std::size_t n) {
T* ptr;
if (n == 0) return NULL;
n = SIZE_ALIGNED(n);
#ifdef USE_CUDA
if (LGBM_config_::current_device == lgbm_device_cuda) {
cudaError_t ret = cudaHostAlloc(&ptr, n*sizeof(T), cudaHostAllocPortable);
if (ret != cudaSuccess) {
Log::Warning("Defaulting to malloc in CHAllocator!!!");
ptr = reinterpret_cast<T*>(_mm_malloc(n*sizeof(T), 16));
}
} else {
ptr = reinterpret_cast<T*>(_mm_malloc(n*sizeof(T), 16));
}
#else
ptr = reinterpret_cast<T*>(_mm_malloc(n*sizeof(T), 16));
#endif
return ptr;
}

@jameslamb
Copy link
Collaborator

Ah got it, thank you! Ok I'll investigate this further. Maybe some headers have changed in newer versions of compilers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants