Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop llvm.compiler.used GV? #2568

Open
MrSidims opened this issue May 17, 2024 · 1 comment
Open

Drop llvm.compiler.used GV? #2568

MrSidims opened this issue May 17, 2024 · 1 comment

Comments

@MrSidims
Copy link
Contributor

MrSidims commented May 17, 2024

Before llvm/llvm-project@1120d8e6f799 compiler FE would place llvm.compiler.used GV in private address space and translator would error out facing it due to recently added diagnostics. Before this diagnostic we were translating it to SPIR-V module to OpVariable with llvm.compiler.used name and function storage class and in SPIRVReader we would drop this GV completely, see https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/lib/SPIRV/SPIRVReader.cpp#L3442 .

Right now with the mentioned community patch translator no longer drop the GV and it survives the translation. But reverse translation would not be 100% correct, we also need to change section of this GV to llvm.metadata. We could add the section, but considering definition from the LangRef:

On targets that support it, this allows an intelligent linker to optimize references to the symbol without being impeded as it would be by @llvm.used.

so strictly speaking, our target here is SPIR-V and SPIR-V doesn't have support for such magic GV, so going to non-necessarily-LLVM-based backend such GV might remain to be unresolved (an we don't have appending linkage type in SPIR-V to help with properly resolving the GV). And IMHO it's better to not generate OpVariable when facing llvm.compiler.used at all also considering that relying on its information passed for optimized from FE to device's compiler is a bad idea. WDYT?

@svenvh
Copy link
Member

svenvh commented May 20, 2024

It seems there is currently no strong use case anyway for these "meta" variables, so dropping them sounds reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants