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

spirv: stop emitting ImageMSArray capability #5663

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

Keenuts
Copy link
Collaborator

@Keenuts Keenuts commented Sep 6, 2023

ImageMSArray is only required is OpTypeImage arrayed=1, MS=1 and sampled=2. This is never the case AFAIK for code emitted from HLSL. No tests or shaders I knew about required it, so should be OK removing it from DXC.

Capability trimming pass is handling this logic now.
Needed to fix a test as DCE now runs.
ImageMSArray is also not required for this test as
no OpImageType has array=1, ms=1 and sampled=2.

Signed-off-by: Nathan Gauër <[email protected]>
@sudonatalie
Copy link
Collaborator

Can you add a test that does require OpCapability ImageMSArray? It looks like this was our only one, so if it wasn't valid we're missing some coverage.

@Keenuts
Copy link
Collaborator Author

Keenuts commented Sep 7, 2023

Can you add a test that does require OpCapability ImageMSArray? It looks like this was our only one, so if it wasn't valid we're missing some coverage.

Yeah, I wanted to do that, but haven't found how. I'll try a bit more, but seems every HLSL code I wrote doesn't have the 1 1 2 pattern in the OpImageType I need for this capability.

@Keenuts Keenuts changed the title spirv: remove logic to add ImageMSArray logic. spirv: stop emitting ImageMSArray capability Sep 8, 2023
@Keenuts
Copy link
Collaborator Author

Keenuts commented Sep 8, 2023

Removed the capability, and fixed the documentation.

@Keenuts Keenuts enabled auto-merge (squash) September 8, 2023 11:25
@Keenuts Keenuts merged commit 3e7c98a into microsoft:main Sep 8, 2023
12 checks passed
@Keenuts
Copy link
Collaborator Author

Keenuts commented Sep 8, 2023

Arf auto-merge

@Keenuts
Copy link
Collaborator Author

Keenuts commented Sep 8, 2023

Let me fix that in another PR

ShchchowAMD pushed a commit to ShchchowAMD/DirectXShaderCompiler that referenced this pull request Sep 11, 2023
ImageMSArray is only required is OpTypeImage arrayed=1, MS=1 and
sampled=2. This is never the case AFAIK for code emitted from HLSL. No
tests or shaders I knew about required it, so should be OK removing it
from DXC.

---------

Signed-off-by: Nathan Gauër <[email protected]>
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.

3 participants