Skip to content

Conversation

@jinge90
Copy link
Contributor

@jinge90 jinge90 commented Sep 26, 2024

Missing "inline" will lead to multiple definition linker error for these trivial functions. The scenario can be described as following:
a.cpp
#include <sycl/sycl.hpp>
#include <sycl/ext/intel/math.hpp>

b.cpp
#include <sycl/sycl.hpp>
#include <sycl/ext/intel/math.hpp>
int main() { return 0; }

clang++ -fsycl a.cpp b.cpp

@jinge90 jinge90 requested a review from a team as a code owner September 26, 2024 05:42
@jinge90
Copy link
Contributor Author

jinge90 commented Sep 26, 2024

Hi, @tangjj11
Could you try this PR in your side to see if the error is gone?
Thanks very much.

@uditagarwal97
Copy link
Contributor

@jinge90 Can you please update the PR description with why this change is needed? What error does it fix?

inline namespace _V1 {
namespace ext::intel::math {
sycl::half hadd(sycl::half x, sycl::half y) { return x + y; }
inline sycl::half hadd(sycl::half x, sycl::half y) { return x + y; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a lit test to build multi file:
a.dp.cpp

#include <sycl/sycl.hpp>
#include <sycl/ext/intel/math.hpp>

b.dp.cpp

#include <sycl/sycl.hpp>
#include <sycl/ext/intel/math.hpp>
int main() { return 0; }

command:

icpx -fsycl a.dp.cpp b.dp.cpp

@jinge90
Copy link
Contributor Author

jinge90 commented Sep 26, 2024

@jinge90 Can you please update the PR description with why this change is needed? What error does it fix?

Hi, @uditagarwal97
Just updated the description, thanks very much.

@uditagarwal97
Copy link
Contributor

@jinge90 Can you please update the PR description with why this change is needed? What error does it fix?

Hi, @uditagarwal97 Just updated the description, thanks very much.

Thanks. The changes looks good to me. Please add a LIT test as suggested by @tangjj11

@jinge90
Copy link
Contributor Author

jinge90 commented Sep 26, 2024

@jinge90 Can you please update the PR description with why this change is needed? What error does it fix?

Hi, @uditagarwal97 Just updated the description, thanks very much.

Thanks. The changes looks good to me. Please add a LIT test as suggested by @tangjj11

Hi, @uditagarwal97
Just split original test to cover separate build scenario.
Thanks very much.

@jinge90 jinge90 requested a review from a team September 27, 2024 04:53
@jinge90
Copy link
Contributor Author

jinge90 commented Sep 27, 2024

Hi, @intel/llvm-gatekeepers
Could you help merge this PR?
Thanks very much.

@steffenlarsen steffenlarsen merged commit 64a9deb into intel:sycl Sep 27, 2024
12 checks passed
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.

4 participants