[SPIR-V] Preserve members after merged bitfields in flat conversions - #8715
[SPIR-V] Preserve members after merged bitfields in flat conversions#8715Diego Novillo (dnovillo) wants to merge 2 commits into
Conversation
A same-type flat conversion must decompose to one scalar per SPIR-V field so that reconstruction consumes the same sequence.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Gregory Roth (@pow2clk) please review. Thanks. |
There was a problem hiding this comment.
Pull request overview
Fixes SPIR-V flat conversion of constant-buffer structs containing merged bitfields, preserving members that follow those bitfields.
Changes:
- Selects decomposition mode based on source and destination AST types.
- Supports decomposing merged bitfields as one physical field.
- Adds regression coverage for copying into a local struct.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tools/clang/lib/SPIRV/SpirvEmitter.cpp |
Implements layout-aware scalar decomposition. |
tools/clang/lib/SPIRV/SpirvEmitter.h |
Updates the decomposition API. |
tools/clang/test/CodeGenSPIRV/type.constant-buffer.fn-var.bitfield.hlsl |
Verifies preservation of post-bitfield members. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Gregory Roth (pow2clk)
left a comment
Gregory Roth (pow2clk)
left a comment
There was a problem hiding this comment.
I hate to harp on a comment, but I had this change all backwards expecting that bitfields were expected to make the comparison mismatch, leading to a true for mergebitfields, which was the existing behavior. I think this comment makes more clear why setting the boolean that way chooses the correct index.
|
Co-authored-by: Gregory Roth <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
Distinct compatible struct types can still shift members following merged bitfields.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| const bool includeMergedBitfields = | ||
| !astContext.hasSameUnqualifiedType(type, sourceType); |
Since I was doing bitfield fixes in #8713, I took this one too.
This PR fixes #8526.
Copying a
ConstantBuffervalue with merged bitfields into a local struct overwrites the members after the bitfields. Forstruct MyData { uint a : 16; uint b : 16; uint c; }, readingcreturns the word holdingaandb.This change makes
processFlatConversionpick the decomposition mode from the source and destination AST types. A same-type conversion emits one scalar per SPIR-V field. But a cast that changes the shape, keeps one scalar per AST field.I added the test from the original issue.