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

Update LongConstantCompositeINTEL to LongCompositesINTEL capability after Headers change #2258

Merged
merged 4 commits into from
Dec 13, 2023

Conversation

vmaksimo
Copy link
Contributor

@vmaksimo vmaksimo commented Dec 8, 2023

Continue #2257
This fixes #2261

The change is needed after KhronosGroup/SPIRV-Registry@5d60c7d and KhronosGroup/SPIRV-Headers@d5acd42

@vmaksimo vmaksimo changed the title Replace internal SPV_INTEL_long_composites with the published SPV_INTEL_long_composites extension Replace SPV_INTEL_long_constant_composite with the updated SPV_INTEL_long_composites extension Dec 8, 2023
@@ -43,7 +43,7 @@ EXT(SPV_INTEL_variable_length_array)
EXT(SPV_INTEL_fp_fast_math_mode)
EXT(SPV_INTEL_fpga_cluster_attributes)
EXT(SPV_INTEL_loop_fuse)
EXT(SPV_INTEL_long_constant_composite)
Copy link
Contributor

Choose a reason for hiding this comment

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

the renaming is a breaking change
please first change SPIRVReader in release branches to accept both SPV_INTEL_long_constant_composite and SPV_INTEL_long_composites, then wait for a graceful period and only then proceed with the renaming in ToT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, thanks!

@vmaksimo vmaksimo changed the title Replace SPV_INTEL_long_constant_composite with the updated SPV_INTEL_long_composites extension Update LongConstantCompositeINTEL to LongCompositesINTEL capability after Headers change Dec 8, 2023
@vmaksimo
Copy link
Contributor Author

Please take a look @asudarsa @LU-JOHN @bwlodarcz @VyacheslavLevytskyy

@VyacheslavLevytskyy
Copy link
Contributor

LGTM

Copy link
Contributor

@LU-JOHN LU-JOHN left a comment

Choose a reason for hiding this comment

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

LGTM

@LW-archlinux
Copy link

This solves #2261 when built against KhronosGroup/SPIRV-Tools@e03c8f5 and KhronosGroup/SPIRV-Headers@1c6bb27

Please add closes: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/issues/2261 or similar to the commit message.

@asudarsa
Copy link
Contributor

Can we please add a TODO in include/LLVMSPIRVExtensions.inc for the required change to happen in the future?

Thanks

Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@asudarsa asudarsa merged commit 0166a0f into KhronosGroup:main Dec 13, 2023
9 checks passed
MrSidims pushed a commit to MrSidims/SPIRV-LLVM-Translator that referenced this pull request Jan 22, 2024
…NTEL capability after Headers change (KhronosGroup#2258)

* Bump SPIRV-Headers to 1c6bb2743599e6eb6f37b2969acc0aef812e32e3
* replace internal SPV_INTEL_long_composites ext with the published SPV_INTEL_long_composites
* don't rename extension for now
This closes: KhronosGroup#2261

Co-authored-by: Wlodarczyk, Bertrand <bertrand.wlodarczyk@intel.com>
MrSidims added a commit that referenced this pull request Jan 23, 2024
…NTEL capability after Headers change (#2258) (#2308)

* Bump SPIRV-Headers to 1c6bb2743599e6eb6f37b2969acc0aef812e32e3
* replace internal SPV_INTEL_long_composites ext with the published SPV_INTEL_long_composites
* don't rename extension for now
This closes: #2261

Co-authored-by: Viktoria Maximova <viktoria.maksimova@intel.com>
Co-authored-by: Wlodarczyk, Bertrand <bertrand.wlodarczyk@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants