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] Spill arrays and matrices for runtime indexing. #6390

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

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Oct 10, 2024

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 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 [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 in
    BlockContext::write_expression_pointer.

  • [naga] Add new function, GuardedIndex::from_expression.

    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.

  • [naga spv-out] Abstract out NumericType::from_inner.

    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.

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

  • [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 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.

Depends on:

@jimblandy jimblandy added type: bug Something isn't working area: naga back-end Outputs of naga shader conversion naga Shader Translator lang: SPIR-V Vulkan's Shading Language labels Oct 10, 2024
@jimblandy jimblandy requested a review from a team as a code owner October 10, 2024 02:05
@jimblandy
Copy link
Member Author

cc @sagudev This is mostly right, I think, but the types on the OpAccessChain instructions are wrong. I think that should be easy to fix.

@jimblandy jimblandy marked this pull request as draft October 10, 2024 02:07
@jimblandy jimblandy force-pushed the naga-spv-out-index-by-value branch 2 times, most recently from c279c58 to bc43c21 Compare October 10, 2024 19:59
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).
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`.
@jimblandy jimblandy marked this pull request as ready for review October 10, 2024 20:00
@jimblandy
Copy link
Member Author

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

@jimblandy jimblandy force-pushed the naga-spv-out-index-by-value branch 4 times, most recently from 9032200 to 32cb10f Compare October 10, 2024 22:53
Let `BlockContext::write_checked_load` take an `AccessTypeAdjustment`
argument, so that the caller can choose what adjustment to apply to
`pointer`.
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.
@jimblandy
Copy link
Member Author

Tweaked comments, and moved out another logically independent change. I think the final commit should be a pretty clear explanation of the idea now.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

@teoxoy
Copy link
Member

teoxoy commented Oct 11, 2024

Should we move the test added by the initial PR into the new snapshot?

https://github.com/gfx-rs/wgpu/pull/6188/files#diff-f63b9d8b161661aa5df1539e321be5f259f9c2e508f3ac970a62f8a050b0fd36

// If `index` is known, we can just use `OpCompositeExtract`.
//
// We never need bounds checks for these cases: everything
// size is statically known and checked in validation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For linking purposes so that we remember to update this in the future: This is not spec compliant but we already have #4390 tracking this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: naga back-end Outputs of naga shader conversion lang: SPIR-V Vulkan's Shading Language naga Shader Translator type: bug Something isn't working
Projects
None yet
2 participants