-
Notifications
You must be signed in to change notification settings - Fork 17
[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
Conversation
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 |
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. |
@pow2clk You mentioned you wanted to take a look at this. Could you review this, and let me know what you think? |
There was a problem hiding this 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.
proposals/NNNN-symbol-visibility.md
Outdated
Then, we can map the HLSL linkages to LLVM IR as follows: | ||
|
||
| HLSL Linkage | LLVM-IR representation | | ||
|----------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
Oh, one more thing, we need a description of the change. A copy of the summary in the document itself is fine. |
I merged the first draft. Feel free to take over from here. Thanks. |
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).