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

Add support for SPV_INTEL_subgroup_requirements #2317

Merged
merged 6 commits into from
Feb 8, 2024

Conversation

jzc
Copy link
Contributor

@jzc jzc commented Jan 25, 2024

Spec: intel/llvm#11301

More accurately, this PR adds support for the named subgroup related features of SPV_INTEL_subgroup_requirements to support implementation of sycl_ext_named_sub_group_sizes (also see intel/llvm#12335). The features related to subgroup lane mapping are not added yet.

@CLAassistant
Copy link

CLAassistant commented Jan 25, 2024

CLA assistant check
All committers have signed the CLA.

lib/SPIRV/PreprocessMetadata.cpp Outdated Show resolved Hide resolved
lib/SPIRV/PreprocessMetadata.cpp Outdated Show resolved Hide resolved
@@ -167,11 +167,21 @@ void PreprocessMetadataBase::visit(Module *M) {

// !{void (i32 addrspace(1)*)* @kernel, i32 35, i32 size}
if (MDNode *ReqdSubgroupSize = Kernel.getMetadata(kSPIR2MD::SubgroupSize)) {
EM.addOp()
// A primary named subgroup size is encoded as
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking, we are changing default behavior here. Right now the translator propagates sub-group size 0 as-is, but with the change applied it will only happen if the extension is enabled.

I would maybe suggest that we preserve the original behavior to avoid accidentally breaking someone's else workflow, i.e. we should only react to 0 if the extension is enabled even at this step, where we only emit some internal metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the behavior and added a test case: 193c25d

Note this code is preprocessing the LLVM module and adding extra execution metadata to that module, so I changed this code to add this metadata in addition to the regular sub group metadata (as opposed to instead). Then in SPIRVWriter, sub group execution mode metadata will be present and be processed like before and the named sub group metadata will be ignored when the extension is turned off. https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/2317/files#diff-ba003da77a80df1db76fb378a873a80e758591d72bf82a46fc90069b2a947990R5838-R5843

@MrSidims
Copy link
Contributor

Restarting CI with a new HEAD

@MrSidims MrSidims closed this Jan 29, 2024
@MrSidims MrSidims reopened this Jan 29, 2024
lib/SPIRV/SPIRVWriter.cpp Outdated Show resolved Hide resolved
@MrSidims MrSidims merged commit 43acfef into KhronosGroup:main Feb 8, 2024
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