[SPIR-V] get field index from SPIR-V type, not AST#4806
[SPIR-V] get field index from SPIR-V type, not AST#4806Keenuts merged 1 commit intomicrosoft:mainfrom
Conversation
Keenuts
changed the title
|
✅ Build DirectXShaderCompiler 1.0.2248 completed (commit 0833101e22 by @Keenuts) |
|
found a bug when implementing bitfields. Removing reviewers |
|
The bug was related to inheritance, when the base class was empty. Because I only used the offset value to determine field merging, I was returning the wrong index for such cases. |
Before this commit, struct field indices were computed from the AST type. This was correct as SPIR-V types and AST types had the same structure. When adding bitfields, we started to squash some fields together, as long as some rules were respected (bitfield, same type, doesn't span over multiple type-sized words, etc). This means indices now diverge between SPIR-V types and AST types. This requires us to lower the AST type before the lowering pass, when generating instructions like OpAccessChain. A cleaner alternative would be to lower all types before instruction generation, and only operate on SPIR-V type. But cost of making this refactoring is large, and we don't believe it is worth it since we plan to upstream to clang (hence rewrite this code). note: this code prevents AST indices to diverge from SPIR-V indices when computed. This is just a safeguard until bitfields are in place in case I made a mistake with the layout rules, allowing us to catch those bugs faster. Signed-off-by: Nathan Gauër <brioche@google.com>
|
✅ Build DirectXShaderCompiler 1.0.2249 completed (commit f76a53ac16 by @Keenuts) |
|
✅ Build DirectXShaderCompiler 1.0.2250 completed (commit a9ff7389cd by @Keenuts) |
cassiebeckley
left a comment
cassiebeckley
left a comment
There was a problem hiding this comment.
Looks great! Thanks for figuring out a way to do this without it being too messy.
|
✅ Build DirectXShaderCompiler 1.0.2251 completed (commit de0a7ae04f by @Keenuts) |
Revert "spirv: get field index from SPIR-V type, not AST (microsoft#4806)" This reverts commit 8f279ba. This commit isn't passing some of our internal tests and needs to be reverted in preparation for the next Vulkan SDK release.
3 participants
Before this commit, struct field indices were computed from the AST type. This was correct as SPIR-V types and AST types had the same structure.
When adding bitfields, we started to squash some fields together, as long as some rules were respected (bitfield, same type, doesn't span over multiple type-sized words, etc).
This means indices now diverge between SPIR-V types and AST types.
This requires us to lower the AST type before the lowering pass, when generating instructions like OpAccessChain.
A cleaner alternative would be to lower all types before instruction generation, and only operate on SPIR-V type. But cost of making this refactoring is large, and we don't believe it is worth it since we plan to upstream to clang (hence rewrite this code).
note: this code prevents AST indices to diverge from SPIR-V indices when computed. This is just a safeguard until bitfields are in place in case I made a mistake with the layout rules, allowing us to catch those bugs faster.
Signed-off-by: Nathan Gauër brioche@google.com