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:🔙:spv::Function`.
Add snapshot test `index-by-value.wgsl`.
Fixes#6358.
Fixes#4337.
Alternative to #6362.
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`.
Pull out the code for building a `naga:🔙:spv::NumericType` from a
`TypeInner` into its own function, `NumericType::from_inner`. Use that
in `LocalType::from_inner`.
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).
Introduce a new function,
`naga:🔙: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.
Delete the function `BlockContext::is_intermediate`. Instead, have
`Access` and `AccessIndex` instructions decide whether to defer code
generation based on the type of the base expression: indexing
operations on pointers are deferred; anything else is not.
Simplify the definition of `naga:🔙:spv::Writer::get_pointer_id`
by using `get_type_id`'s ability to handle `LocalType::Pointer`.
This means that `get_pointer_id` is no longer fallible, and no longer
needs a type arena. Simplify callers, as well as the
`BlockContext::get_pointer_id` convenience function.
In `back::spv`:
- Factor out the numeric variants of `LocalType` into a
new enum, `NumericType`.
- Split the `Value` variant into `Numeric` and `LocalPointer`
variants, and let `LocalPointer` point to any numeric type,
including matrices.
In subsequent commits, we'll need to spill matrices out into temporary
local variables. This means we'll need to generate SPIR-V
pointer-to-matrix types, so `LocalType` needs to be able to represent
that.
Do not neglect to apply bounds checks to indexing operations on
runtime-sized arrays, even when they are accessed via an `AccessIndex`
instruction.
Before this commit, `BlockContext::write_expression_pointer` would not
apply bounds checks to `OpAccessChain` indices provided by an
`AccessIndex` instruction, apparently with the rationale that any
out-of-bounds accesses should have been reported by constant
evaluation.
While it is true that the `index` operand of an `AccessIndex`
expression is known at compile time, and that the WGSL constant
evaluation rules require accesses that can be statically determined to
be out-of-bounds to be shader creation or pipeline creation time
errors, accesses to runtime-sized arrays don't follow this pattern:
even if the index is known, the length with which it must be compared
is not.
Fixes#4441.
Let `BoundsCheckResult::Conditional` provide both the condition to
check before carrying out the access, and the index to use for that
access. The `Conditional` variant indicates that we generated a
runtime bounds check, which implies we must have had a SPIR-V id for
the index to pass to that check, so there's no reason not to provide
that to the callers - especially if the bounds check code was able to
reduce it to a known constant.
At the moment, this is not much of a refactor, but later commits will
use `GuardedIndex` in more places, at which point this will avoid a
re-matching and assertion.
Abstract the code from `write_expression_pointer` to handle one
indexing operation out into its own function,
`BlockContext::write_access_chain_index`.
Let the SPIR-V backend use `GuardedIndex::try_resolve_to_constant`,
rather than writing out its definition in `write_restricted_index` and
`write_index_comparison`.
Call `try_resolve_to_constant` in one place, in `write_bounds_check`,
and simply pass the `GuardedIndex` into subroutines.
Reduce `write_restricted_index` and `write_index_comparison` to case
analysis and code generation.
Note that this commit does have a benign effect on SPIR-V snapshot
output for programs like this:
let one_i = 1i;
var vec0 = vec3<i32>();
vec0[one_i] = 1;
The value indexing `vec0` here is an `i32`, but after this commit, the
operand to `OpAccessChain` becomes a `u32` constant (with the same
value).
This is because `write_bounds_check` now calls
`try_resolve_to_constant` itself, rather than deferring this work to
its callees, so it may return `BoundsCheckResult::KnownInBounds` even
when the `Unchecked` policy is in force. This directs the caller,
`write_expression_pointer`, to treat the `OpAccessChain` operand as a
fresh `u32` constant, rather than simply passing through the original
`i32` expression.
Introduce a new function,
`BlockContext::extend_bounds_check_condition_chain`, which adds a new
boolean condition to the chain of bounds checks guarding an
`OpAccessChain` instruction.
Introduce a new function,
`BlockContext::is_nonuniform_binding_array_access`, which determines
whether a given array access expression means that the `OpAccessChain`
instruction must have a `NonUniform` decoration.
When generating SPIR-V, avoid generating unreachable blocks following
statements like `break`, `return`, and so on that cause non-local
exits. These unreachable blocks can cause SPIR-V validation to fail.
Fixes#6220.
Make `naga:🔙:spv::BlockContext::write_block` private to
`naga:🔙:spv::block`. Introduce a new `pub(super)` function,
`write_function_body`, for `Writer::write_function` to call.
Make `BlockExit` private to `naga:🔙:spv::block`.
Move `LoopContext` from `naga:🔙:spv` into
`naga:🔙:spv::block`, and make it private.
Consolidate the explanation of why and how we wrap Naga global
variables in structs to satisfy Vulkan's requirements, and include it
in the documentation for `back::spv::GlobalVariable`.
Clarify `GlobalVariable`'s members documentation.
Bring https://github.com/gfx-rs/naga/pull/723 back from the dead.
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Co-authored-by: Jim Blandy <jimb@red-bean.com>
Document and refactor
`naga:🔙:spv::BlockContext::write_runtime_array_length`.
Don't try to handle finding the length of a particular element of a
`binding_array<array<T>>`. The SPIR-V backend doesn't wrap that type
correctly anyway; #6333 changes the validator to forbid such types.
Instead, assume that the elements of a `binding_array<T>` are always
structs whose final members may be a runtime-sized array.
Pull out consistency checks after the analysis of the array
expression, so that we always carry out all the checks regardless of
what path we took to produce the information.
Require `T` to be a struct in `binding_array<T, ...>`; do not permit
arrays.
In #5428, the validator was changed to accept binding array types that
the SPIR-V backend couldn't properly emit. Specifically, the validator
was changed to accept `binding_array<array<T>>`, but the SPIR-V
backend wasn't changed to wrap the binding array elements in a SPIR-V
struct type, as Vulkan requires. So the type would be accepted by the
validator, and then rejected by the backend.
`Validator::validate_module_handles` already ensures that types refer
only to other types appearing earlier in the arena than themselves, so
this check in `Validator::validate_type` is redundant.
See the comments in the code for details.
This patch emits the definition of the macro only when the first loop
is encountered. This does make that first loop's code look a bit odd:
it would be more natural to define the macro at the top of the
file. (See the modified files in `naga/tests/out/msl`.)
Rejected alternatives:
- We could emit the macro definition unconditionally at the top of the
file. But this changes every MSL snapshot output file, whereas only
eight of them actually contain loops.
- We could have the validator flag modules that contain loops. But the
changes end up being not small, and spread across the validator, so
this seems disproportionate. If we had other consumers of this
information, it might make sense.
- We could change the MSL backend to allow text to be generated out of
order, so that we can decide whether to define the macro after we've
generated all the function bodies. But at the moment this seems like
unnecessary complexity, although it might be worth doing in the
future if we had additional uses for it - say, to conditionally emit
helper function definitions.
Fixes#4972.
