Map LinkOnceODR to weak_odr instead of linkonce_odr #2502
Conversation
The translator currently maps SPIR-V's `LinkOnceODR` to llvm's `linkonce_odr`, but that allows llvm to discard unreferenced globals. 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.
|
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 |
There was a problem hiding this comment.
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?
|
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. |
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. |
The translator currently maps SPIR-V's
LinkOnceODRto llvm'slinkonce_odr, but that allows llvm to discard unreferenced globals, causing linking failures when linkinglinkonce_odrsymbols from multiple modules. llvm'sweak_odrhas the same merging semantics as llvm'slinkonce_odr, except that unreferenced globals may not be discarded.SPIR-V's
LinkOnceODRis "Same as theExportlinkage 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 withExportlinkage (that gets mapped tocommonorexternalllvm linkage) are not discarded, soweak_odrseems to be a better fit forLinkOnceODR.When producing SPIR-V, map both
linkonce_odrandweak_odrtoLinkOnceODRfor backwards compatibility, and add an llvm function with the previouslinkonce_odrlinkage to the test.