Add workgroupBarrier to WGSL #1449
Conversation
Fixes gpuweb#1374 * Adds workgroupBarrier as a control barrier templated on affected storage classes * Modifies func_call_statement grammar to allow limited templates
wgsl/index.bs
Outdated
| atomic operation program ordered after the workgroupBarrier is executed by a | ||
| member of the workgroup. | ||
|
|
||
| A memory, atomic or workgroupBarrier operation is an afftected operation if it |
There was a problem hiding this comment.
Do we have a description of what a "memory" operation is? I.e. is loading from a storage texture a memory operation?
There was a problem hiding this comment.
If read_write textures are eventually added, then barriers would need to be updated to allow the handle storage class, and yes, would be considered memory operations. To me, the right place to describe memory operations would be in the memory model section which is still TODO; however, we could say that memory operations are a read or write to a variable (or through a pointer) in an affected storage class if that is preferable.
There was a problem hiding this comment.
Hold on. So today without read/write textures, if I do two different textureStore operations, there are no barriers I could use to ensure that one is done after the other?
There was a problem hiding this comment.
Correct. Metal doesn't add image barriers widely until Metal 2.0 so I didn't want to force that requirement on WGSL. I think that without read_write textures, the problem you describe isn't that relevant. It would mean you're overwriting the same pixel from multiple invocations without any interim reads. I don't think that's the common case.
I'd be happy to add texture coverage if we knew only Metal 2.0 or later was being targeted with WGSL.
There was a problem hiding this comment.
If we are to add "handle" here, then it will be only place in the grammar where "handle" is recognized, which is somewhat strange/inelegant. We'll need to eithe:
- consider
handlestorage class being optional on textures/samplers (to make the spec consistent) - switch this
workgroupBarrierto not work with the storage classes directly but instead have another way of specifying the mask, i.e. it could be even something as:
struct MemoryBarrier {
storage: bool,
handle: bool,
workgroup: bool,
};
fn workgroupBarrier(barrier: MemoryBarrier) {..}I think there is still value in trying to keep the existing grammar rules for this operation rather than inventing an entirely new syntax...
|
I'm happy to iterate on barriers if there is a better alternative syntax. I personally don't want pre-defined structs. Constexprs would be ideal, but even without them we could potentially require literal u32 value that acted as a bitmask. |
* Remove templated function and replace with two control barriers * workgroupBarrier - affects memory and atomics in workgroup * storageBarrier - affects memory and atomics in storage * both barriers synchroize with each other
|
The latest commit implements the changes discussed in the VF2F 2021-02-23 (my commit message has the wrong date). |
| memory ordering. That is, all synchronization functions, and affected memory | ||
| and atomic operations are ordered in [[#program-order]] relative to the | ||
| synchronization function. Additionally, the affected memory and atomic | ||
| operations program-ordered before the synchronization function must be visible |
There was a problem hiding this comment.
I'll followup with a rewording for editorial purposes.
Fixes #1374
storage classes