-
Notifications
You must be signed in to change notification settings - Fork 246
fix: capturing deep composites #874
base: main
Are you sure you want to change the base?
Conversation
eddyb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo comments, but I am a bit confused: is it actually possible for us to end up with OpCompositeInserts with multiple indices? Do we have a pass that generates those?
(I'm asking because AFAIK, rustc_codegen_ssa sure doesn't have the ability to emit anything similar, at most it does single-level and usually only for ScalarPair anyway)
crates/rustc_codegen_spirv/src/linker/destructure_composites.rs
Outdated
Show resolved
Hide resolved
crates/rustc_codegen_spirv/src/linker/destructure_composites.rs
Outdated
Show resolved
Hide resolved
crates/rustc_codegen_spirv/src/linker/destructure_composites.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Eduard-Mihai Burtescu <[email protected]>
Co-authored-by: Eduard-Mihai Burtescu <[email protected]>
Co-authored-by: Eduard-Mihai Burtescu <[email protected]>
Co-authored-by: Eduard-Mihai Burtescu <[email protected]>
I suspect it's if a closure captures an storage buffer ( |
That's true at the type level, but is there something that generates the equivalent of |
it seems when I simplify my reproduction the error is gone, but my original shader does have multi index CompositeInsert. So the testcase isn't correct. Will try to get a new testcase if possible |
try fixing #873 . context is this #690 (review) .
The tests I added still errors, but a different one compared to what I am trying to fix.
But the wired thing is, if I compile the tests using my own setup in my own repo, it compiles successfully, but in the tests it fails with a different error. Also this change fixes my original problem (in my own repo).