[SPIR-V] Fix vk::BufferPointer bitfield stores - #8713
Conversation
Preserve the bitfield AST type and align the generated load and store.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@pow2clk I can't add reviewers yet. Could you please review? |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
pow2clk
left a comment
pow2clk
left a comment
There was a problem hiding this comment.
I tried to convince myself that there was some way to reorganize things so the alignment value could be calculated sooner and the bitfieldLoad wouldn't have to be outside the conditional so the bitfield handling code could be consolidated, but I don't think that's reasonable if it is possible.
There was a problem hiding this comment.
Pull request overview
Fixes a SPIR-V codegen crash when storing into a bitfield through vk::BufferPointer by ensuring the replacement value keeps AST type information so alignment can be computed and applied to the generated memory ops.
Changes:
- Preserve AST result type on the bitfield-inserted store value to avoid a null/invalid type flowing into alignment calculation.
- Apply the computed PhysicalStorageBuffer alignment to both the generated load (for read-modify-write) and the final store.
- Add a regression test covering
pc.ptr.Get().a = 123for a bitfield member.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/clang/lib/SPIRV/SpirvBuilder.cpp | Keeps AST typing/alignment intact for bitfield RMW stores via vk::BufferPointer, preventing the alignment calculator crash. |
| tools/clang/test/CodeGenSPIRV/vk.buffer-pointer.bitfield.hlsl | Adds a regression test asserting the expected OpLoad/OpBitFieldInsert/OpStore sequence with Aligned 4. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bitfieldLoad = createLoad(value->getResultType(), address, loc, range); | ||
| source = createBitFieldInsert(/*QualType*/ {}, bitfieldLoad, value, | ||
| bitfieldInfo->offsetInBits, | ||
| bitfieldInfo->sizeInBits, loc, range); | ||
| source->setResultType(value->getResultType()); | ||
| source->setAstResultType(value->getAstResultType()); |
|
This fixes #8402.
Storing to a bitfield through
vk::BufferPointercrashes inAlignmentSizeCalculator::getAlignmentAndSize. The bitfield update was losing the AST Type and alignment information.This change preserves the original AST type while producing the replacement value. It calculates the physical-memory alignment from that type and applies it to both the generated load and store.
I added the original reproducer as a new test.