Add support for zero-initializing workgroup memory #2111

teoxoy · 2022-11-03T14:41:57Z

closes #53

Possible future optimizations:

spread out initialization across workgroup (most useful for arrays)
detect user initialization to avoid double initialization

src/back/msl/writer.rs

ErichDonGubler

Not finished reviewing yet, but so far, this LGTM. I'll see if I can finish this before end-of-week.

nitpick(non-blocking): 1k LoC in a single commit is a lot. I do think there's some opportunity for breaking this up, but fortunately, the candidates for splitting commits look easy (which tends to mean that the impact isn't high, thankfully). I'll try to call them out where I can.

question: Isn't this sort of new functionality the sort of thing we'd want a CHANGELOG entry for? I see that we don't currently have a PR template encouraging this, which might be something I can do a la wgpu.

note: RE: the Path warning: this warning has been fixed with 0197246, which you should catch with a rebase. 🙌🏻

src/back/glsl/mod.rs

ErichDonGubler · 2022-12-27T17:08:37Z

src/back/spv/writer.rs

@@ -65,6 +65,7 @@ impl Writer {
            annotations: vec![],
            flags: options.flags,
            bounds_check_policies: options.bounds_check_policies,
+            zero_initialize_workgroup_memory: options.zero_initialize_workgroup_memory,


thought(non-blocking): I feel like using pattern destructuring for data forwarding would be a really good fit for this sort of logic in Self::new and Self::reset. I'm the type that doesn't trust myself without that sort of safety net, tho, so YMMV. 😅

I don't use destructuring much as I like seeing where the var came from at first glance.
@jimblandy thoughts?

In this particular case I kind of wonder if Writer shouldn't just store an Options directly, instead of copying out all of its members. (If someone wants to do this, let's make it a separate PR.)

In the general case, it's true that destructuring requires that every field gets mentioned, and introduces locals that rustc will complain if we don't use—whereas it can't complain about an unread struct field if it's pub. And I definitely like to take advantage of these static safety nets - I'll often ask people to take out _ => ... arms from match statements just so we can be sure that newly variants get consideration at each use.

I think in this case, for Options, it doesn't pay off, primarily because there's only one spot where the Options value is consulted, and it's the same spot that all the other fields are used. So pretty much any implementation flow that works at all is going to arrive at the spot that needs attention. In other words, it doesn't seem especially error-prone. This wouldn't be the case for, say, a new TypeInner variant: those are used all over the place. Or a new BoundsCheckPolicy - those are consumed in a handful of places scattered around each backend.

If I'm going to talk about things 'paying off', then I should say what the cost is: I think it takes a bit longer to read the destructuring version, there are more variables around that could be mixed with other things, and as Teo said, it's less obvious, when looking at a use, what its origin was.

That is, when I see options.bounds_check_policies, I know exactly where it came from, but when I see just bounds_check_policies, it could be an argument, it could have been computed, etc. and I have to guess or go find out.

jimblandy · 2023-01-04T14:38:24Z

Possible future optimizations:

* spread out initialization across workgroup (most useful for arrays)

* detect user initialization to avoid double initialization

These are good ideas - could you file an issue for each?

jimblandy

I haven't looked at the SPIR-V back end yet, but I do have one change I want to see, so I'll just post what I have now.

I'd also be happy to see a TypeFlags-based revision, if you want.

src/back/glsl/mod.rs

src/back/hlsl/writer.rs

src/back/msl/writer.rs

jimblandy · 2023-01-04T21:27:23Z

Sorry, some of those comments were intended as replies to Erich's questions, but GitHub didn't see it that way so they're kind of out-of-context.

teoxoy · 2023-01-05T10:57:39Z

@ErichDonGubler

nitpick(non-blocking): 1k LoC in a single commit is a lot. I do think there's some opportunity for breaking this up, but fortunately, the candidates for splitting commits look easy (which tends to mean that the impact isn't high, thankfully). I'll try to call them out where I can.

Most changes seem to come from the test snapshots. Will move the barrier code refactor to a separate commit.

question: Isn't this sort of new functionality the sort of thing we'd want a CHANGELOG entry for? I see that we don't currently have a PR template encouraging this, which might be something I can do a la wgpu.

Tbh, I prefer adding the changelog entries when we do a release since I think it results in higher quality changelogs but what could also have the same effect is asking authors to add a changelog entry (same as wgpu) and doing a last pass before a release to make sure the changelog is cohesive.

jimblandy

This changed the API for backends, so it needs a CHANGELOG.md entry.

Don't all these writes need to be limited to a single invocation? Right now we're writing to all this storage from every invocation, and that's going to be a race and a waste.

src/back/msl/writer.rs

jimblandy · 2023-01-06T16:35:33Z

src/back/spv/writer.rs

@@ -65,6 +65,7 @@ impl Writer {
            annotations: vec![],
            flags: options.flags,
            bounds_check_policies: options.bounds_check_policies,
+            zero_initialize_workgroup_memory: options.zero_initialize_workgroup_memory,


In this particular case I kind of wonder if Writer shouldn't just store an Options directly, instead of copying out all of its members. (If someone wants to do this, let's make it a separate PR.)

In the general case, it's true that destructuring requires that every field gets mentioned, and introduces locals that rustc will complain if we don't use—whereas it can't complain about an unread struct field if it's pub. And I definitely like to take advantage of these static safety nets - I'll often ask people to take out _ => ... arms from match statements just so we can be sure that newly variants get consideration at each use.

I think in this case, for Options, it doesn't pay off, primarily because there's only one spot where the Options value is consulted, and it's the same spot that all the other fields are used. So pretty much any implementation flow that works at all is going to arrive at the spot that needs attention. In other words, it doesn't seem especially error-prone. This wouldn't be the case for, say, a new TypeInner variant: those are used all over the place. Or a new BoundsCheckPolicy - those are consumed in a handful of places scattered around each backend.

src/back/msl/writer.rs

src/back/spv/mod.rs

jimblandy

I think the only remaining question here is the one I raised above: should we restrict initialization to a single invocation?

src/back/spv/writer.rs

tests/out/glsl/workgroup-var-init.main.Compute.glsl

teoxoy · 2023-01-09T18:47:09Z

This changed the API for backends, so it needs a CHANGELOG.md entry.

Yeah, the question is if we want to require updates to the changelog as part of PRs or do it ourselves before a release.

Don't all these writes need to be limited to a single invocation? Right now we're writing to all this storage from every invocation, and that's going to be a race and a waste.

Indeed, pushed a new commit.

jimblandy

Still need to process the SPIR-V changes, but I did find something in the MSL.

src/back/hlsl/writer.rs

src/back/msl/writer.rs

teoxoy · 2023-01-23T11:31:29Z

@jimblandy I addressed the latest comments.

jimblandy

One problem in the validator changes. I'll look at this again first thing tomorrow morning.

src/valid/type.rs

tests/out/analysis/collatz.info.ron

tests/in/padding.param.ron

jimblandy

Looks good!

jimblandy · 2023-01-25T04:02:12Z

src/back/spv/writer.rs

+                Instruction::variable(pointer_type_id, varying_id, class, None)
+                    .to_words(&mut self.logical_layout.declarations);
+
+                self.decorate(
+                    varying_id,
+                    spirv::Decoration::BuiltIn,
+                    &[spirv::BuiltIn::GlobalInvocationId as u32],
+                );


I understand why, but it's a shame that we have to replicate code like this. This is an example of a case where lowering the Naga IR to some intermediate representation before generating SPIR-V would simplify things. But that shouldn't be addressed in this PR.

ErichDonGubler mentioned this pull request Nov 3, 2022

Zero-initialize workgroup memory gfx-rs/wgpu#3174

Merged

3 tasks

teoxoy force-pushed the zero_init_workgroup_mem branch from 6bc913d to 9cc3bec Compare November 3, 2022 17:43

teoxoy commented Nov 3, 2022

View reviewed changes

src/back/msl/writer.rs Outdated Show resolved Hide resolved

teoxoy requested a review from jimblandy November 4, 2022 09:13

ErichDonGubler reviewed Dec 27, 2022

View reviewed changes

jimblandy suggested changes Jan 4, 2023

View reviewed changes

teoxoy force-pushed the zero_init_workgroup_mem branch 2 times, most recently from 835c055 to e57e951 Compare January 5, 2023 12:49

jimblandy reviewed Jan 6, 2023

View reviewed changes

jimblandy suggested changes Jan 6, 2023

View reviewed changes

src/back/spv/writer.rs Outdated Show resolved Hide resolved

src/back/spv/writer.rs Outdated Show resolved Hide resolved

tests/out/glsl/workgroup-var-init.main.Compute.glsl Outdated Show resolved Hide resolved

teoxoy force-pushed the zero_init_workgroup_mem branch from e57e951 to dfae06f Compare January 9, 2023 18:37

teoxoy force-pushed the zero_init_workgroup_mem branch from dfae06f to 5f2f940 Compare January 9, 2023 18:51

teoxoy requested a review from jimblandy January 9, 2023 18:54

jimblandy suggested changes Jan 20, 2023

View reviewed changes

src/back/hlsl/writer.rs Outdated Show resolved Hide resolved

src/back/msl/writer.rs Outdated Show resolved Hide resolved

teoxoy force-pushed the zero_init_workgroup_mem branch from 5f2f940 to 3708326 Compare January 23, 2023 11:30

jimblandy suggested changes Jan 24, 2023

View reviewed changes

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

tests/out/analysis/collatz.info.ron Outdated Show resolved Hide resolved

tests/in/padding.param.ron Show resolved Hide resolved

teoxoy force-pushed the zero_init_workgroup_mem branch from 3708326 to 623988c Compare January 24, 2023 10:49

jimblandy approved these changes Jan 25, 2023

View reviewed changes

teoxoy added 5 commits January 25, 2023 17:54

factor out write_barrier functions

c1ed459

[hlsl-out] simplify write_default_init

787c8e6

add type_flags to ModuleInfo

0aad863

fix array being flagged as constructible when its base isn't

2317661

add support for zero-initializing workgroup memory

827512c

teoxoy force-pushed the zero_init_workgroup_mem branch from 0458e45 to 827512c Compare January 25, 2023 17:00

teoxoy merged commit c7d0215 into gfx-rs:master Jan 25, 2023

teoxoy deleted the zero_init_workgroup_mem branch January 25, 2023 17:07

Patryk27 mentioned this pull request Jul 5, 2023

Initializing workgroup memory EmbarkStudios/rust-gpu#1077

Open

rust-gpu-bot mentioned this pull request Nov 13, 2024

[Migrated] Initializing workgroup memory Rust-GPU/rust-gpu#80

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for zero-initializing workgroup memory #2111

Add support for zero-initializing workgroup memory #2111

teoxoy commented Nov 3, 2022 •

edited

Loading

ErichDonGubler left a comment

ErichDonGubler Dec 27, 2022

teoxoy Jan 5, 2023

jimblandy Jan 6, 2023 •

edited

Loading

jimblandy Jan 6, 2023

jimblandy commented Jan 4, 2023

jimblandy left a comment

jimblandy commented Jan 4, 2023

teoxoy commented Jan 5, 2023

jimblandy left a comment

jimblandy Jan 6, 2023 •

edited

Loading

jimblandy left a comment

teoxoy commented Jan 9, 2023

jimblandy left a comment

teoxoy commented Jan 23, 2023

jimblandy left a comment

jimblandy left a comment

jimblandy Jan 25, 2023

Add support for zero-initializing workgroup memory #2111

Add support for zero-initializing workgroup memory #2111

Conversation

teoxoy commented Nov 3, 2022 • edited Loading

ErichDonGubler left a comment

Choose a reason for hiding this comment

ErichDonGubler Dec 27, 2022

Choose a reason for hiding this comment

teoxoy Jan 5, 2023

Choose a reason for hiding this comment

jimblandy Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

jimblandy Jan 6, 2023

Choose a reason for hiding this comment

jimblandy commented Jan 4, 2023

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy commented Jan 4, 2023

teoxoy commented Jan 5, 2023

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

teoxoy commented Jan 9, 2023

jimblandy left a comment

Choose a reason for hiding this comment

teoxoy commented Jan 23, 2023

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy Jan 25, 2023

Choose a reason for hiding this comment

teoxoy commented Nov 3, 2022 •

edited

Loading

jimblandy Jan 6, 2023 •

edited

Loading

jimblandy Jan 6, 2023 •

edited

Loading