Map LinkOnceODR to weak_odr instead of linkonce_odr

[svenvh]

The translator currently maps SPIR-V's LinkOnceODR to llvm's linkonce_odr, but that allows llvm to discard unreferenced globals, causing linking failures when linking linkonce_odr symbols from multiple modules. llvm's weak_odr has the same merging semantics as llvm's linkonce_odr, except that unreferenced globals may not be discarded.

SPIR-V's LinkOnceODR is "Same as the Export linkage type but a function or global variable with this linkage type will be merged with other functions or global variables of the same name when linkage occurs". Unreferenced globals with Export linkage (that gets mapped to common or external llvm linkage) are not discarded, so weak_odr seems to be a better fit for LinkOnceODR.

When producing SPIR-V, map both linkonce_odr and weak_odr to LinkOnceODR for backwards compatibility, and add an llvm function with the previous linkonce_odr linkage to the test.

[asudarsa]

Is there a reason why we named the SPIR-V linkage LinkOnceODR and not WeakODR in the first place? May be we will benefit from adding a new linkage (WeakODR)? Adding @bashbaug for comments. Thanks

[MrSidims]

The patch make sense, but it also feels like a specification bug as the intention for predecessor Intel extension was made to express linkonce_odr without considering weak_odr. Regardless, the patch should be merged as it follows the specification, but may I ask to hold it for a day?

[svenvh]

Thanks for the comments so far. No problem holding off merging this PR; it would be great to hear what @bashbaug or @Fznamznon think.

[MrSidims]

Thanks for the comments so far. No problem holding off merging this PR; it would be great to hear what @bashbaug or @Fznamznon think.

I don't mind to merge, worst case we will revert it downstream, as it's a consumer part, not generator.

[svenvh]

I don't mind to merge, worst case we will revert it downstream

We're not in a rush for this, so we could wait a few more days for folks to step in. I'm quite keen to hear some more thoughts.

[AlexVlx]

Is there a reason why we named the SPIR-V linkage LinkOnceODR and not WeakODR in the first place? May be we will benefit from adding a new linkage (WeakODR)? Adding @bashbaug for comments. Thanks

I was under the impression that the initial motivation for adding the extension was precisely to support LLVM's semantics, which in turn model the C++ ones (https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_linkonce_odr.asciidoc). Since weak / weak_odr are explicitly called out as distinct in the extension's design doc, perhaps this aliasing would be confusing. It would also seem that Export naturally maps to external in LLVM, and not weak (weak is somewhat special purpose). These are not the same as, for the former, if no definition is found, you'll get a link time error, whereas for the latter you would not and could run into surprising behaviour.