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

[naga spv-out] bounds-check runtime-sized array access correctly. #6351

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Commits on Sep 30, 2024

  1. [naga spv-out] Abstract out non-uniform binding array access test.

    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.
    jimblandy committed Sep 30, 2024
    Configuration menu
    Copy the full SHA
    1f902c4 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    798b492 View commit details
    Browse the repository at this point in the history
  3. [naga spv-out] Abstract extending a bounds check condition chain.

    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.
    jimblandy committed Sep 30, 2024
    Configuration menu
    Copy the full SHA
    c829d67 View commit details
    Browse the repository at this point in the history

Commits on Oct 1, 2024

  1. [naga spv-out] Consolidate code to find index values.

    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.
    jimblandy committed Oct 1, 2024
    Configuration menu
    Copy the full SHA
    99514dd View commit details
    Browse the repository at this point in the history
  2. [naga spv-out] Abstract out OpAccessChain index production.

    Abstract the code from `write_expression_pointer` to handle one
    indexing operation out into its own function,
    `BlockContext::write_access_chain_index`.
    jimblandy committed Oct 1, 2024
    Configuration menu
    Copy the full SHA
    a1bc031 View commit details
    Browse the repository at this point in the history
  3. [naga spv-out] Let BoundsCheckResult supply the index value.

    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.
    jimblandy committed Oct 1, 2024
    Configuration menu
    Copy the full SHA
    b3d60e0 View commit details
    Browse the repository at this point in the history
  4. [naga] Extend snapshot tests for bounds checks.

    Extend the snapshot tests for bounds checking to cover the case where
    a runtime-sized array is indexed by a constant.
    jimblandy committed Oct 1, 2024
    Configuration menu
    Copy the full SHA
    64bdaf4 View commit details
    Browse the repository at this point in the history
  5. [naga spv-out] Bounds-check runtime-sized array access correctly.

    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 gfx-rs#4441.
    jimblandy committed Oct 1, 2024
    Configuration menu
    Copy the full SHA
    cc134f1 View commit details
    Browse the repository at this point in the history