[SYCL][libdevice] Don't force linkage to weak

[sarnex]

I'm working on thinLTO and weak symbols are not guaranteed to be the same everywhere, so the thinLTO infrastructure does not link them in so devices libraries don't work.

I don't know of any reason to explicitly mark them as weak and in reality these functions should follow ODR, so just remove the specification.

[againull]

Looks good to me. Adding @jinge90 for awareness just in case if there are some concerns.

[sarnex]

Thanks, I will wait a bit for any feedback from @jinge90 before merging.

[jinge90]

Hi, @sarnex and @againull
When sycl device library was added, we found some user may define their own version, for example, they may define a device version "sinf/cosf" in source code and multiple definition error will be reported since sycl device library also has provided these functions. Since sycl device library are linked by default, we don't know whether user will define their own version of some library function, so "weak" is added to avoid link error.
I haven't tried the latest compiler but suspect compiler will fail to compile any code including their own definition for functions which are provided in sycl device library as well if we remove "weak".
Thanks very much.

[sarnex]

@jinge90 Thanks for the response. It seems like the relevant SYCL spec document is here

It seems the problem in question could happen with separate translation units, one that does not include cmath and uses a user defined sinf, and another one that includes cmath and uses the device library sinf.

My understanding of the current behavior/design as explained above is that we explicitly mark all device lib functions as weak linkage so that in the above case, both translation units will end up using the user defined sinf when linked.

This seems really strange to me. My initial reaction is that an error is the correct behavior, overwriting the function from another TU seems weird.

@gmlueck @intel/dpcpp-specification-reviewers Can you guys comment if the SYCL spec and/or our SYCL extensions give an idea of what should happen in this case? If not, can you give your opinion on what should happen?

[gmlueck]

This isn't really a spec question. The spec does not say that you can define your own version of a standard library. Strictly speaking, I think this is UB by the C++ language. However, if customers are doing it now, and you cause that code to break, then those customers will be unhappy.

What is the motivation for removing the "weak"? It sounds like thinLTO doesn't work with weak symbols? However, weak symbols are a pretty common C/C++ feature. Does thinLTO not work for regular C/C++ programs when there are weak symbols?

It seems the problem in question could happen with separate translation units, one that does not include cmath and uses a user defined sinf, and another one that includes cmath and uses the device library sinf.

I think @jinge90 is saying that the problem occurs even with a single TU that defines a a function like sinf. It would be good to try some examples and see exactly what cases will break if you remove the "weak".

don't want this to be merged accidentally

[sarnex]

Thanks Greg. It looks like it actually thinLTO with weak symbols does work for pure x86 code, so let me investigate why the upstream community clang-linker-wrapper thinLTO doesn't work but the upstream community x86 thinLTO linker plugin does work. I'll come back to this PR if I think we really do need this change. Thanks to everyone for giving me a better place to investigate. Marking as a draft for now.

[github-actions]

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

[github-actions]

This pull request was closed because it has been stalled for 30 days with no activity.