Skip to content

[spirv] Added handling for multi-dimensional CT buffers#4507

Merged
miseydl merged 1 commit intomicrosoft:mainfrom
miseydl:multi_dim_cts
Jun 14, 2022
Merged

[spirv] Added handling for multi-dimensional CT buffers#4507
miseydl merged 1 commit intomicrosoft:mainfrom
miseydl:multi_dim_cts

Conversation

@miseydl
Copy link
Copy Markdown
Contributor

@miseydl miseydl commented Jun 10, 2022

Previously the usage of multi dimensional ConstantBuffers would have just crashed the SPIRV generation due to a flawed assumption. This implementation is unrolling the array and is emitting the correct SPIRV.

Remark: When generating SPIRV for any Vulkan target environment the validation will fail due to a violation of Vulkan spec section 14.5.2, which requires uniform buffers to be of type struct or "array of struct". Apparently "array of array of struct" is upsetting the validator.

@ghost
Copy link
Copy Markdown

ghost commented Jun 10, 2022

CLA assistant check
All CLA requirements met.

@AppVeyorBot
Copy link
Copy Markdown

@miseydl miseydl closed this Jun 10, 2022
@miseydl miseydl reopened this Jun 10, 2022
@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Could you add a test (under tools/clang/test/CodeGenSPIRV)? I recently opened an issue with a minimized repro that could serve as a test case: #4506

Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
@kuhar kuhar added the spirv Work related to SPIR-V label Jun 13, 2022
@miseydl
Copy link
Copy Markdown
Contributor Author

miseydl commented Jun 14, 2022

Thanks for working on this!

Could you add a test (under tools/clang/test/CodeGenSPIRV)? I recently opened an issue with a minimized repro that could serve as a test case: #4506

Test has been added. I took your repo almost verbatim.

Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/lib/SPIRV/DeclResultIdMapper.cpp Outdated
Comment thread tools/clang/test/CodeGenSPIRV/type.constant-buffer.multiple-dimensions.hlsl Outdated
Comment thread tools/clang/test/CodeGenSPIRV/type.constant-buffer.multiple-dimensions.hlsl Outdated
Comment thread tools/clang/test/CodeGenSPIRV/type.constant-buffer.multiple-dimensions.hlsl Outdated
@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Collaborator

@kuhar kuhar 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 for the changes.

One tiny nit: could you prepend [spirv] to the commit / PR title?

@miseydl miseydl changed the title Added handling for multi-dimensional CT buffers [spirv] Added handling for multi-dimensional CT buffers Jun 14, 2022
@miseydl
Copy link
Copy Markdown
Contributor Author

miseydl commented Jun 14, 2022

LGTM. Thanks for the changes.

One tiny nit: could you prepend [spirv] to the commit / PR title?

Thanks for reviewing!

@AppVeyorBot
Copy link
Copy Markdown

@AppVeyorBot
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

spirv Work related to SPIR-V

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants