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.

Sign up for GitHub

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

Jump to bottom

opt: add Int16 and Float16 to capability trim pass #5519

Merged
merged 1 commit into from
Jan 4, 2024
Merged

opt: add Int16 and Float16 to capability trim pass #5519

merged 1 commit into from
Jan 4, 2024

Conversation

Keenuts
Copy link
Contributor

@Keenuts Keenuts commented Jan 4, 2024 •
edited

Add support for Int16 and Float16 trim.

Fixes #5464

@Keenuts
opt: add Int16 and Float16 to capability trim pass
09f1605 
Add support for Int16 and Float16 trim.

Signed-off-by: Nathan Gauër <brioche@google.com>
@Keenuts Keenuts mentioned this pull request Jan 4, 2024
Closed
@sudonatalie
Copy link
Collaborator

If I remember correctly, the Float16 capability is sometimes provided by an extension (not sure about Int16). Do we also want to also trim any unused extensions as part of this pass?

@Keenuts
Copy link
Contributor Author

Keenuts commented Jan 4, 2024

If I remember correctly, the Float16 capability is sometimes provided by an extension (not sure about Int16). Do we also want to also trim any unused extensions as part of this pass?

Float16 is provided by an extension? I see the vulkan extension, but not a SPIR-V one adding this. Isn't that core SPV 1.0?

For extensions: the pass supports trimming extensions. This is done using the grammar. If no operand/opcode requires an extension, it is marked as non-required, and trimmed.
If there is such extension, I should add a test, but support should be there (in theory).

@sudonatalie
Copy link
Collaborator

If I remember correctly, the Float16 capability is sometimes provided by an extension (not sure about Int16). Do we also want to also trim any unused extensions as part of this pass?

Float16 is provided by an extension? I see the vulkan extension, but not a SPIR-V one adding this. Isn't that core SPV 1.0?

For extensions: the pass supports trimming extensions. This is done using the grammar. If no operand/opcode requires an extension, it is marked as non-required, and trimmed. If there is such extension, I should add a test, but support should be there (in theory).

Hm, I was thinking of AMD_gpu_shader_half_float but it looks like it's only necessary for float16 support of certain extended instructions and is just incorrectly documented in SPIR-V.rst. I'll fix our docs.

sudonatalie added a commit to sudonatalie/DirectXShaderCompiler that referenced this pull request Jan 4, 2024
@sudonatalie
[SPIR-V] Remove dead code and correct Float16 docs
4c023ce 
The docs incorrectly stated that the `SPV_AMD_gpu_shader_half_float`
extension was added when half types were used. This is now corrected to
show that the Float16 capability is added instead.

The remaining code that could have added that extension in certain cases
has been dead code since microsoft#2503 since none of the instructions checked in
that list are ones that are emitted by DXC, so it is removed to clean
up.

Follow up from KhronosGroup/SPIRV-Tools#5519
@sudonatalie sudonatalie mentioned this pull request Jan 4, 2024
Merged
sudonatalie added a commit to sudonatalie/DirectXShaderCompiler that referenced this pull request Jan 4, 2024
@sudonatalie
[SPIR-V] Remove dead code and correct Float16 docs
6779c30 
The docs incorrectly stated that the `SPV_AMD_gpu_shader_half_float`
extension was added when half types were used. This is now corrected to
show that the Float16 capability is added instead.

The remaining code that could have added that extension in certain cases
has been dead code since microsoft#2503 since none of the instructions checked in
that list are ones that are emitted by DXC, so it is removed to clean
up.

Follow up from KhronosGroup/SPIRV-Tools#5519
@Keenuts Keenuts merged commit c7affa1 into KhronosGroup:main Jan 4, 2024
24 checks passed
@Keenuts Keenuts deleted the add-16bit-types branch January 4, 2024 19:01
sudonatalie added a commit to microsoft/DirectXShaderCompiler that referenced this pull request Jan 4, 2024
@sudonatalie
[SPIR-V] Remove dead code and correct Float16 docs (#6127)
1574879 
The docs were out of date stating that the
`SPV_AMD_gpu_shader_half_float` extension was added when half types were
used. This is now corrected to show that the Float16 capability is added
and no extension.

The remaining code that could have added that extension in certain cases
has been dead code since #2503 since none of the instructions checked in
that list are ones that are emitted by DXC, so it is removed to clean
up.

Evidence that this is dead code:
1. `git grep "InterpolateAt"` finds no uses other than those removed
here.
2. All tests still pass when it's removed.
3. The comments in #2503 also say they were not able to find a mapping
from any HLSL intrinsics to these instructions, which is why no tests
were added.

Follow-up from: KhronosGroup/SPIRV-Tools#5519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sudonatalie sudonatalie sudonatalie approved these changes

@s-perron s-perron s-perron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Capability Stripping Pass doesn't seem to strip Int16 or Float16
3 participants
@Keenuts @sudonatalie @s-perron