Skip to content

Always enable SPIR-V#7221

Closed
pow2clk wants to merge 7 commits intomicrosoft:mainfrom
pow2clk:disable_disble_spirv
Closed

Always enable SPIR-V#7221
pow2clk wants to merge 7 commits intomicrosoft:mainfrom
pow2clk:disable_disble_spirv

Conversation

@pow2clk
Copy link
Copy Markdown
Collaborator

@pow2clk pow2clk commented Mar 18, 2025

disables the ability to disable SPIR-V. This allows intermediate opcodes to be consistent at least across the same compiler build regardless of build environment. This is a requirement for IR tests that depend on these opcodes.

Given the embrace of SPIR-V and the stability the generation target has gained, there's no reason not to do this.

Fixes #7169

disables the ability to disable SPIR-V. This allows intermediate opcodes
to be consistent at least across the same compiler build regardless of
build environment. This is a requirement for IR tests that depend on
these opcodes.

Given the embrace of SPIR-V and the stability the generation target has
gained, there's no reason not to do this.

Fixes microsoft#7169
@pow2clk
Copy link
Copy Markdown
Collaborator Author

pow2clk commented Mar 18, 2025

Note that I've enabled this by force rather than just enabling it by default to address existing installs. This does mean that some will have to get the SPIRV subdirs to get what they need, but the alternative is seemingly failing IR tests

might be a problem since some of this is generated code
@pow2clk
Copy link
Copy Markdown
Collaborator Author

pow2clk commented Mar 18, 2025

format checker is refusing to find he latest hash. null change incoming

@pow2clk
Copy link
Copy Markdown
Collaborator Author

pow2clk commented Mar 18, 2025

Well, there seems to be no satisfying the formatting bot for the present.

Copy link
Copy Markdown
Member

@damyanp damyanp left a comment

Choose a reason for hiding this comment

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

LGTM - would it also be good to get a review from some of the developers working on SPIR-V as well?

Is there any documentation in the repo that needs updating? READMEs etc?

@pow2clk
Copy link
Copy Markdown
Collaborator Author

pow2clk commented Mar 19, 2025

I assure you this was clang-formatted. The bot is having a bad week. Bypassing.

@pow2clk
Copy link
Copy Markdown
Collaborator Author

pow2clk commented Mar 19, 2025

LGTM - would it also be good to get a review from some of the developers working on SPIR-V as well?

Is there any documentation in the repo that needs updating? READMEs etc?

I didn't see this comment before my last comment for security reasons. These are good suggestions.

@pow2clk
Copy link
Copy Markdown
Collaborator Author

pow2clk commented Mar 24, 2025

Given that we have another way to solve the problem of different configurations with the same build failing IR test due to instable opcode values with #7231, suddenly requiring the SPIRV submodules upon syncing to the latest is no longer the lesser evil as tests will no longer suddenly fail on SPIRV disabled builds. As such, we are still determined to change the default to enabling SPIRV, but it will be just a few lines changing defaults in cmake and batch files.

This change doesn't bear sufficient resemblance to that eventual simple, though impactful change, so I'm closing it.

@pow2clk pow2clk closed this Mar 24, 2025
@github-project-automation github-project-automation Bot moved this from New to Done in HLSL Roadmap Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[Feature Request] Always enable SPIR-V

5 participants