-
Notifications
You must be signed in to change notification settings - Fork 970
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
[naga spv-out] Spill arrays and matrices for runtime indexing. #6390
[naga spv-out] Spill arrays and matrices for runtime indexing. #6390
Conversation
cc @sagudev This is mostly right, I think, but the types on the |
bc43c21
to
e479b15
Compare
@sagudev I think this is ready to test. In the end, I think this wasn't simpler than yours, but I don't think it's too hard to understand, and I got a bunch of nice cleanups in there in the process. |
32cb10f
to
d741a89
Compare
Tweaked comments, and moved out another logically independent change. I think the final commit should be a pretty clear explanation of the idea now. |
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.
This looks great!
Should we move the test added by the initial PR into the new snapshot? |
Introduce a new function, `naga::back::spv::BlockContext::write_checked_load`, that does the work of `Expression::Load`. This change is just code motion, and should have no effect on behavior. The new function will be used in later commits.
Replace the `return_type_override` argument of `BlockContext::write_expression_pointer` with an enum that says how to derive the return type from `expr_handle`'s type. Introduce a new type, `AccessTypeAdjustment`, that covers possible derivation rules. This simplifies callers and the callee, in part by making the possible alternatives less general, and by giving them explicit names (the variants of the `AccessTypeAdjustment` enum).
This commit is just code motion.
Replace qualified paths with a `use` directive.
Pull out the code for building a `naga::back::spv::NumericType` from a `TypeInner` into its own function, `NumericType::from_inner`. Use that in `LocalType::from_inner`.
Pull out the code to build a `naga::proc::index::GuardedIndex` from a `Handle<Expression>` into its own function, `GuardedIndex::from_expression`. Use that function in `GuardedIndex::try_resolve_to_constant`.
Introduce a new helper function, `naga::back::spv::Writer::get_resolution_pointer_id`. Use it in `BlockContext::write_expression_pointer`.
Let `BlockContext::write_checked_load` take an `AccessTypeAdjustment` argument, so that the caller can choose what adjustment to apply to `pointer`.
fdce2de
to
5689cb4
Compare
Improve handling of `Access` expressions whose base is an array or matrix (not a pointer to such), and whose index is not known at compile time. SPIR-V does not have instructions that can do this directly, so spill such values to temporary variables, and perform the accesses using `OpAccessChain` instructions applied to the temporaries. When performing chains of accesses like `a[i].x[j]`, do not reify intermediate values; generate a single `OpAccessChain` for the entire thing. Remove special cases for arrays; the same code now handles arrays and matrices. Update validation to permit dynamic indexing of matrices. For details, see the comments on the new tracking structures in `naga::back::spv::Function`. Add snapshot test `index-by-value.wgsl`. Fixes gfx-rs#6358. Fixes gfx-rs#4337. Alternative to gfx-rs#6362.
5689cb4
to
9dc3691
Compare
I tested in servo and it works as expected🎉. |
When we implemented dynamic indexing of matrices in gfx-rs#6390, we stopped needing to recognize types that could only be indexed by a constant expression. However, we accidentally left a `false` in the `match` expression, even though it's no longer used. Replace that with a `{}` for clarity.
I tried to break this up into easy-to-review commits, but ymmv.
[naga spv-out] Spill arrays and matrices for runtime indexing.
Improve handling of
Access
expressions whose base is an array ormatrix (not a pointer to such), and whose index is not known at
compile time. SPIR-V does not have instructions that can do this
directly, so spill such values to temporary variables, and perform the
accesses using
OpAccessChain
instructions applied to thetemporaries.
When performing chains of accesses like
a[i].x[j]
, do not reifyintermediate values; generate a single
OpAccessChain
for the entirething.
Remove special cases for arrays; the same code now handles arrays and
matrices.
Update validation to permit dynamic indexing of matrices.
For details, see the comments on the new tracking structures in
naga::back::spv::Function
.Add snapshot test
index-by-value.wgsl
.Fixes [naga spv-out] Indexing arrays by value causes unnecessary copies #6358.
Fixes Support dynamic indexing of by-value matrices and arrays, per WGSL #4337.
Alternative to [naga] Support dynamic indexing of by-value matrices #6362.
[naga spv-out] Add some tracing output to
Writer::write_function
.[naga spv-out] Introduce
Writer::get_resolution_pointer_id
.Introduce a new helper function,
naga::back::spv::Writer::get_resolution_pointer_id
. Use it inBlockContext::write_expression_pointer
.[naga] Add new function,
GuardedIndex::from_expression
.Pull out the code to build a
naga::proc::index::GuardedIndex
from aHandle<Expression>
into its own function,GuardedIndex::from_expression
. Use that function inGuardedIndex::try_resolve_to_constant
.[naga spv-out] Abstract out
NumericType::from_inner
.Pull out the code for building a
naga::back::spv::NumericType
from aTypeInner
into its own function,NumericType::from_inner
. Use thatin
LocalType::from_inner
.[naga spv-out] Use
crate::proc::index::GuardedIndex
.Replace qualified paths with a
use
directive.[naga spv-out] Gather array, matrix, and vector cases.
This commit is just code motion.
[naga] Test access to a member/element through a pointer.
[naga spv-out] Clean up
write_expression_pointer
type adjustment.Replace the
return_type_override
argument ofBlockContext::write_expression_pointer
with an enum that says how toderive the return type from
expr_handle
's type.Introduce a new type,
AccessTypeAdjustment
, that covers possiblederivation rules.
This simplifies callers and the callee, in part by making the possible
alternatives less general, and by giving them explicit names (the
variants of the
AccessTypeAdjustment
enum).[naga spv-out] Move code to load a pointer into its own function.
Introduce a new function,
naga::back::spv::BlockContext::write_checked_load
, that does thework of
Expression::Load
.This change is just code motion, and should have no effect on
behavior. The new function will be used in later commits.
Depends on:
Writer::get_pointer_id
. #6388BlockContext::is_intermediate
; use types. #6389