Skip to content

Conversation

idubinov
Copy link
Contributor

@idubinov idubinov commented Sep 15, 2025

Global constants cannot be processed in SPIRV. This change enables the translation of global constants declared in the private address space into local variables within the functions where they are used

@CLAassistant
Copy link

CLAassistant commented Sep 15, 2025

CLA assistant check
All committers have signed the CLA.

@idubinov idubinov changed the title Fix global const Allow translation of global internal constants with private address space Sep 16, 2025
Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but I'd like to hear opinion from other reviewers as well.

@@ -0,0 +1,46 @@
; RUN: llvm-as %s -o %t.bc
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add a scenario, when 2 (or more) internal constant GVs are used within the same function. We want to make sure, that alloca's are placed consequently in the entry basic basic block, preceding any other instruction (including generated by the added code store instructions).
Reason: from https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_logical_layout_of_a_module

All OpVariable instructions in a function must be in the first block in the function. These instructions, together with any intermixed OpLine and OpNoLine instructions, must be the first instructions in that block. (Note the validation rules prevent OpPhi instructions in the first block of a function.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@MrSidims MrSidims left a comment

Choose a reason for hiding this comment

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

Also, may be a good idea to do this conditionally under a new llvm-spirv option, like "internalize-global-constants", but I don't have a strong opinion on that, defaulting internalization like it's done in the patch should be safe. WDYT @svenvh ?

@svenvh
Copy link
Member

svenvh commented Sep 18, 2025

Also, may be a good idea to do this conditionally under a new llvm-spirv option, like "internalize-global-constants", but I don't have a strong opinion on that, defaulting internalization like it's done in the patch should be safe. WDYT @svenvh ?

No strong opinion on gating this behind an option; the patch as it is now seems fine to me.

@MrSidims MrSidims merged commit 667e1cb into KhronosGroup:main Sep 18, 2025
9 checks passed
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.

4 participants