Support array bindings of buffers #2282

kvark · 2023-03-16T06:51:08Z

Built upon #2256 , which should land first. I can also move this one out (so far just one commit).
Only WGSL -> IR -> SPV path supported right now.
Closes #1864

codecov-commenter · 2023-03-18T06:04:08Z

Codecov Report

Merging #2282 (a5d2d09) into master (53d62b9) will increase coverage by 0.00%.
The diff coverage is 95.16%.

❗ Current head a5d2d09 differs from pull request most recent head 7866e97. Consider uploading reports for the commit 7866e97 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #2282   +/-   ##
=======================================
  Coverage   82.19%   82.19%           
=======================================
  Files          82       82           
  Lines       44267    44296   +29     
=======================================
+ Hits        36384    36410   +26     
- Misses       7883     7886    +3

Impacted Files	Coverage Δ
src/back/spv/mod.rs	`95.23% <ø> (ø)`
src/valid/type.rs	`90.48% <80.00%> (-0.18%)`	⬇️
src/back/spv/block.rs	`91.41% <94.73%> (-0.07%)`	⬇️
src/back/spv/helpers.rs	`96.66% <100.00%> (+0.05%)`	⬆️
src/back/spv/writer.rs	`96.74% <100.00%> (+0.02%)`	⬆️
src/proc/index.rs	`90.66% <100.00%> (+0.06%)`	⬆️
src/proc/typifier.rs	`77.41% <100.00%> (+0.08%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jimblandy

Just a few comments so far:

src/proc/typifier.rs

jimblandy · 2023-04-12T03:14:50Z

src/valid/type.rs

+                if base >= handle {
+                    return Err(TypeError::UnresolvedBase(base));
+                }


We don't need these checks any more, since they're all handled up front in valid/handles.rs.

We are still doing these checks for Ti::Pointer and Ti::Array, so this one is there for consistency. If we want to remove them all, that's fine by me, but should be done separately.

We do want to remove them all, since they're redundant. There's no reason to make more work for the person who has to clean that up. Filed as #2303.

This doesn't need to be removed in this PR if you don't want. It's not like we'll be able to miss it when we fix #2303.

CI seems to now be failing due to this since we already merged #2308.

src/valid/type.rs

jimblandy

I see why TypeFlags::DATA is needed, but I'd also like us to pass this test, in the wgsl-errors.rs style:

#[test]
fn binding_array_local() {
    check_validation! {
        "fn f() { var x: binding_array<i32, 4>; }":
        Err(_)
    }
}

kvark · 2023-04-14T04:16:01Z

@jimblandy take a look at the new commit. We aren't putting DATA on the binding arrays any more.

jimblandy · 2023-04-19T04:31:52Z

How about this one:

#[test]
fn binding_array_private() {
    check_validation! {
        "var<private> x: binding_array<i32, 4>;":
        Err(_)
    }
}

jimblandy

I think that test case I posted above shows that validate_global_var can't quite treat a binding_array<T> just like a T, although there's good reason to think that's usually the right thing.

src/valid/interface.rs

jimblandy · 2023-04-19T05:27:44Z

Is this comment on BlockContext::write_expression_pointer in back/spv/block.rs still accurate?

    /// Some cases we need to generate a different return type than what the IR gives us.
    /// This is because pointers to binding arrays don't exist in the IR, but we need to
    /// create them to create an access chain in SPIRV.

Because, we certainly do generate pointers to binding arrays in the IR now.

jimblandy

Other than these issues, and the additional test I mentioned above, this looks good to go.

src/valid/type.rs

src/back/spv/mod.rs

src/back/spv/block.rs

kvark · 2023-04-24T03:52:07Z

@jimblandy thanks for suggestions! New validation error is added, tests improved, and documentation patches applied.

…ructs

jimblandy

Thank you!

* Support buffer resource arrays in IR, wgsl-in, and spv-out * spv-out: refactor non-uniform indexing semantics to support buffers * Update the doc comment on BindingArray type * Improve TypeInfo restrictions on binding arrays * Strip DATA out of binding arrays * Include suggested documentation, more binding array tests, enforce structs

kvark requested a review from cwfitzgerald March 16, 2023 06:51

kvark force-pushed the storage-array branch from 8c5cabe to 247f8c5 Compare March 18, 2023 05:55

kvark force-pushed the storage-array branch 5 times, most recently from faf65b0 to 777f5e5 Compare March 18, 2023 22:34

kvark mentioned this pull request Mar 22, 2023

Ray Query support #2256

Merged

5 tasks

kvark force-pushed the storage-array branch 3 times, most recently from a5d2d09 to 7866e97 Compare March 23, 2023 03:40

jimblandy reviewed Apr 12, 2023

View reviewed changes

kvark mentioned this pull request Apr 12, 2023

Storage buffers in binding arrays aren't writable #1864

Closed

jimblandy reviewed Apr 13, 2023

View reviewed changes

src/valid/type.rs Outdated Show resolved Hide resolved

jimblandy reviewed Apr 13, 2023

View reviewed changes

kvark force-pushed the storage-array branch 3 times, most recently from 728e41e to a067ea5 Compare April 18, 2023 04:45

jimblandy requested changes Apr 19, 2023

View reviewed changes

src/valid/interface.rs Show resolved Hide resolved

jimblandy requested changes Apr 19, 2023

View reviewed changes

src/valid/type.rs Show resolved Hide resolved

src/back/spv/mod.rs Show resolved Hide resolved

src/back/spv/block.rs Show resolved Hide resolved

kvark force-pushed the storage-array branch from 44b45db to 74dbb64 Compare April 24, 2023 04:15

kvark added 4 commits April 24, 2023 20:18

Support buffer resource arrays in IR, wgsl-in, and spv-out

a0c5a08

spv-out: refactor non-uniform indexing semantics to support buffers

fffb7e6

Update the doc comment on BindingArray type

bb63aaf

Improve TypeInfo restrictions on binding arrays

d1f2ff7

kvark added 2 commits April 24, 2023 20:18

Strip DATA out of binding arrays

22e738d

Include suggested documentation, more binding array tests, enforce st…

d7e8075

…ructs

kvark force-pushed the storage-array branch from 74dbb64 to d7e8075 Compare April 25, 2023 03:20

jimblandy approved these changes Apr 25, 2023

View reviewed changes

jimblandy merged commit 37b3c36 into gfx-rs:master Apr 25, 2023

kvark deleted the storage-array branch April 25, 2023 17:58

kvark added the can backport PR that can be back-ported to a release branch label May 24, 2023

kvark mentioned this pull request May 30, 2023

[0.12] Support array bindings of buffers #2361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support array bindings of buffers #2282

Support array bindings of buffers #2282

kvark commented Mar 16, 2023 •

edited

Loading

codecov-commenter commented Mar 18, 2023 •

edited

Loading

jimblandy left a comment

jimblandy Apr 12, 2023

kvark Apr 12, 2023

jimblandy Apr 12, 2023

jimblandy Apr 12, 2023

teoxoy Apr 24, 2023

jimblandy left a comment

kvark commented Apr 14, 2023

jimblandy commented Apr 19, 2023 •

edited

Loading

jimblandy left a comment

jimblandy commented Apr 19, 2023 •

edited

Loading

jimblandy left a comment •

edited

Loading

kvark commented Apr 24, 2023

jimblandy left a comment

Support array bindings of buffers #2282

Support array bindings of buffers #2282

Conversation

kvark commented Mar 16, 2023 • edited Loading

codecov-commenter commented Mar 18, 2023 • edited Loading

Codecov Report

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy Apr 12, 2023

Choose a reason for hiding this comment

kvark Apr 12, 2023

Choose a reason for hiding this comment

jimblandy Apr 12, 2023

Choose a reason for hiding this comment

jimblandy Apr 12, 2023

Choose a reason for hiding this comment

teoxoy Apr 24, 2023

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

kvark commented Apr 14, 2023

jimblandy commented Apr 19, 2023 • edited Loading

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy commented Apr 19, 2023 • edited Loading

jimblandy left a comment • edited Loading

Choose a reason for hiding this comment

kvark commented Apr 24, 2023

jimblandy left a comment

Choose a reason for hiding this comment

kvark commented Mar 16, 2023 •

edited

Loading

codecov-commenter commented Mar 18, 2023 •

edited

Loading

jimblandy commented Apr 19, 2023 •

edited

Loading

jimblandy commented Apr 19, 2023 •

edited

Loading

jimblandy left a comment •

edited

Loading