[ESIMD] Add 'inline' to function decls to fix ODR problems.

[kbobrovs]

Fixes "multiple definition" linker errors when multiple sources using ESIMD are linked together.

Signed-off-by: Konstantin S Bobrovsky [email protected]

[kychendev]

With this change, for non-sycl-device build, compiler FE will see one SYCL_EXTERNAL declaration and one inline definition for the same API, while the existing code made it consistent. What is the exact FE rule here, or compiler doesn't have diagnostic for this? Should similar change be made in other files such as esimd_math_intrin.hpp?

[kbobrovs]

inline will be seen only by the host compiler, which is really the intent.
Device compiler will continue to see SYCL_EXTERNAL w/o definition

[kbobrovs]

Should similar change be made in other files such as esimd_math_intrin.hpp?

Yes. This is achieved by including inline into ESIMD_INLINE used in other files.
Also, SYCL_EXTERNAL is defined to empty string for the host compiler, so void foo followed by inline void foo should be OK.

[kbobrovs]

Had a sync with @kychendev - esimd_math_intrin.hpp should be really fixed, my omission. Will do.

[kbobrovs]

It turns out all the intrinsic functions in esimd_math_intrin.hpp are templated. C++ ODR allows to have multiple definitions for template functions in a program as long as they are not specializations (always full for functions) and appear in different
translation units (+ some more conditions, which are all satisfied in our case). See https://en.cppreference.com/w/cpp/language/definition

One Definition Rule
...
There can be more than one definition in a program of each of the following: class type, enumeration type, inline function, inline variable (since C++17), templated entity (template or member of template, but not full template specialization), as long as all of the following is true:

So there is no need to fix esimd_math_intrin.hpp, even though due to a different reason than I provided before.
@kychendev - please approve if you are OK with this resolution.

[kbobrovs]

So there is no need to fix esimd_math_intrin.hpp

actually, some clean-up still would be good - SYCL_EXTERNAL is used for host-only definitions, which does not make sense, albeit harmless. Plus the test has an error, which prevents from correct execution - it is still good for intended purpose, but I'll fix it anyway.

[kychendev]

@kbobrovs, looks good but there is test error related to unsupported sycl target. Please double check.

[kbobrovs]

@kychendev, all checks now pass

[kbobrovs]

@bader, please take a look.

[bader]

@bader, please take a look.

@kbobrovs, I'm familiar with this code very well, so I think your approval as a code owner should be enough to merge this patch.

[kbobrovs]

@bader, please take a look.

@kbobrovs, I'm familiar with this code very well, so I think your approval as a code owner should be enough to merge this patch.

@bader, right, but in this case I'm also an author. But if you are OK with this, I will merge myself then. Thanks.

[bader]

But if you are OK with this, I will merge myself then. Thanks.

I suggest adding one more person as a code owner for ESIMD, who is familiar with the code and can do code review for your changes.
I'm okay, but it doesn't count. :-)

[kbobrovs]

I suggest adding one more person as a code owner for ESIMD

right, good idea. I created #3114

[kbobrovs]

@DenisBakhvalov - could you please review/approve? @kychendev already approved (approval lost due to test re-formatting)