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 pointer types in vector when using extension SPV_INTEL_masked_gather_scatter #6041

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

VishMCW
Copy link

@VishMCW VishMCW commented Mar 17, 2025

current checks in spirv-val only check for vector types that are scalar, on using extension SPV_INTEL_masked_gather_scatter OpTypeVector must also support pointer types
check spec: SPV_INTEL_masked_gather_scatter

@CLAassistant
Copy link

CLAassistant commented Mar 17, 2025

CLA assistant check
All committers have signed the CLA.

@VishMCW VishMCW closed this Mar 17, 2025
@VishMCW VishMCW reopened this Mar 17, 2025
@VishMCW VishMCW force-pushed the spv_intel_masked_gather_scatter branch 2 times, most recently from f47a194 to 79c0a9c Compare March 17, 2025 10:12
Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

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

Please add a test for this change.

@VishMCW VishMCW force-pushed the spv_intel_masked_gather_scatter branch from 79c0a9c to 568c47b Compare April 1, 2025 09:31
@VishMCW VishMCW requested a review from alan-baker April 1, 2025 09:32
Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

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

Also please fix the formatting errors.

bool isPointer = component_type->opcode() == spv::Op::OpTypePointer;
bool isScalar = spvOpcodeIsScalarType(component_type->opcode());

if (_.HasExtension(Extension::kSPV_INTEL_masked_gather_scatter) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I think strictly speaking this should be capability gated by the spec:

Suggested change
if (_.HasExtension(Extension::kSPV_INTEL_masked_gather_scatter) &&
if (_.HasCapability(spv::Capability::MaskedScatterGatherINTEL) &&

Copy link
Author

Choose a reason for hiding this comment

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

I have made the changes to use capability instead of extension

<< _.getIdName(component_id)
<< ": Expected a scalar or pointer type when using the "
"SPV_INTEL_masked_gather_scatter extension.";
} else if (!_.HasExtension(Extension::kSPV_INTEL_masked_gather_scatter) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Suggested change
} else if (!_.HasExtension(Extension::kSPV_INTEL_masked_gather_scatter) &&
} else if (!_.HasCapability(spv::Capability::MaskedScatterGatherINTEL) &&

MaskedGatherScatterINTEL is enabled
Add supporting tests for spv_intel_masked_gather_scatter
Formatting changes for unrelated files
@VishMCW VishMCW force-pushed the spv_intel_masked_gather_scatter branch from 568c47b to 62c6ecf Compare April 1, 2025 18:25
@VishMCW VishMCW requested a review from alan-baker April 1, 2025 18:27
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