search

KhronosGroup / SPIRV-LLVM-Translator

A tool and a library for bi-directional translation between SPIR-V and LLVM IR
Other
468 stars 209 forks source link

Map LinkOnceODR to weak_odr instead of linkonce_odr #2502

Open svenvh opened 4 months ago

svenvh commented 4 months ago

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 commented 4 months ago

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

svenvh commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.