Skip to content

[0026] Add proposal for symbol visibility. #272

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

Merged
merged 6 commits into from
May 14, 2025
Merged

Conversation

s-perron
Copy link
Collaborator

@s-perron s-perron commented May 6, 2025

Section 3.6 of the
HLSL specification
defined the possible linkages for names. This proposal updates how these
linkages are represented in LLVM IR. The current implementation presents
challenges for the SPIR-V backend due to inconsistencies with OpenCL. In HLSL, a
name can have external linkage and program linkage, among others. If a name has
external linkage, it is visible outside the translation unit, but not outside a
linked program. A name with program linkage is visible outside a partially linked program.
We propose that names with program linkage in HLSL should have external linkage
and default visibility in LLVM IR, while names with external linkage in HLSL should have
external linkage and hidden visibility in LLVM IR. They both have external
linkage because they are visible outside the translation unit. Default
visibility means the name is visible outside a shared library (program). Hidden
visibility means the name is not visible outside the shared library (program).

@s-perron s-perron requested a review from pow2clk May 6, 2025 15:53
@bogner
Copy link
Collaborator

bogner commented May 6, 2025

This seems sensible to me. Presumably we'll still need the finalize linkage pass in the DirectX backend to drop unused functions with hidden visibility, and we may need something similar in the SPIR-V backend in order to leave these out of the final SPIR-V module - is that correct? Either way, representing these semantics using the existing LLVM constructs and getting rid of the custom hlsl.external metadata seems like a net positive.

@s-perron
Copy link
Collaborator Author

s-perron commented May 6, 2025

Presumably we'll still need the finalize linkage pass in the DirectX backend to drop unused functions with hidden visibility, and we may need something similar in the SPIR-V backend in order to leave these out of the final SPIR-V module - is that correct?

Yes that is correct. However, now it can be implemented in the SPIR-V backend without changing OpenCL behaviour. When we add an HLSL linker to the tools chain, it can do that itself.

@s-perron
Copy link
Collaborator Author

@pow2clk You mentioned you wanted to take a look at this. Could you review this, and let me know what you think?

Copy link
Collaborator

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In spite of all my suggestions, I think this is in a state sufficient to be merged if @s-perron wants to hand it off. I think we discussed me taking it over at that point. Regardless, I'm happy to make my proposed suggestions myself if that's more convenient.

Then, we can map the HLSL linkages to LLVM IR as follows:

| HLSL Linkage | LLVM-IR representation |
|----------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't always insist on the 80 column limit, but this is too wide. I recommended adding explanations of the HLSL linkage terms above. Perhaps they can be moved there and these could just reference them for more detail. That or the rationale can be listed in a following paragraph. Or both!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I like information in the table. When looking at table it draws the eye, and makes it easy to find what information you want, and the connection.

However, I have trouble formatting tables with lots of content. The mark down formater I uses a syntax that GitHub does not understand to split a cell up over multiple lines. I'll leave it like this for now because I am not sure what to do with it.

@pow2clk
Copy link
Collaborator

pow2clk commented May 13, 2025

Oh, one more thing, we need a description of the change. A copy of the summary in the document itself is fine.

@s-perron s-perron changed the title Add proposal for symbol visibility. [0026] Add proposal for symbol visibility. May 14, 2025
@s-perron s-perron merged commit b46fa42 into llvm:main May 14, 2025
@s-perron
Copy link
Collaborator Author

I think this is in a state sufficient to be merged if @s-perron wants to hand it off.

I merged the first draft. Feel free to take over from here. Thanks.

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

Successfully merging this pull request may close these issues.

3 participants