Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: capturing deep composites #874

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

molikto
Copy link
Contributor

@molikto molikto commented May 17, 2022

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).

@molikto molikto requested a review from eddyb as a code owner May 17, 2022 02:29
Copy link
Contributor

@eddyb eddyb left a 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)

@eddyb eddyb added the s: waiting on author PRs that blocked on the author implementing feedback. label May 21, 2022
@eddyb eddyb self-assigned this May 21, 2022
molikto and others added 5 commits May 22, 2022 10:15
Co-authored-by: Eduard-Mihai Burtescu <edy.burt@gmail.com>
Co-authored-by: Eduard-Mihai Burtescu <edy.burt@gmail.com>
Co-authored-by: Eduard-Mihai Burtescu <edy.burt@gmail.com>
Co-authored-by: Eduard-Mihai Burtescu <edy.burt@gmail.com>
@molikto
Copy link
Contributor Author

molikto commented May 22, 2022

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)

I suspect it's if a closure captures an storage buffer (&[u32]) and then another acceleration structure, the closure capturing generate a top level composite, then the array is another composite (len and pointer)

@eddyb
Copy link
Contributor

eddyb commented May 22, 2022

I suspect it's if a closure captures an storage buffer (&[u32]) and then another acceleration structure, the closure capturing generate a top level composite, then the array is another composite (len and pointer)

That's true at the type level, but is there something that generates the equivalent of .field.subfield extracts/inserts in a single SPIR-V instruction? (as opposed to what rustc_codegen_ssa might generate for such independent operations, of one instruction for .field and one for .subfield - tho it usually only does pointer offsets, not even by-value extract/insert)

@molikto
Copy link
Contributor Author

molikto commented May 23, 2022

I suspect it's if a closure captures an storage buffer (&[u32]) and then another acceleration structure, the closure capturing generate a top level composite, then the array is another composite (len and pointer)

That's true at the type level, but is there something that generates the equivalent of .field.subfield extracts/inserts in a single SPIR-V instruction? (as opposed to what rustc_codegen_ssa might generate for such independent operations, of one instruction for .field and one for .subfield - tho it usually only does pointer offsets, not even by-value extract/insert)

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

@eddyb eddyb added the s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) label Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: qptr may fix This might be fixed by the "qptr" experiment (https://github.com/EmbarkStudios/spirt/pull/24) s: waiting on author PRs that blocked on the author implementing feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants