Skip to content

Adjusts subgroup built-in function names to match GLSL#4528

Closed
Lichtso wants to merge 1 commit intogpuweb:mainfrom
Lichtso:main
Closed

Adjusts subgroup built-in function names to match GLSL#4528
Lichtso wants to merge 1 commit intogpuweb:mainfrom
Lichtso:main

Conversation

@Lichtso
Copy link
Copy Markdown

@Lichtso Lichtso commented Mar 22, 2024

We are already very close to the GLSL naming scheme. I don't see any good reason why we should diverge from it for just a few function names.

Adjusts built-in function names to match GLSL.
Copy link
Copy Markdown
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

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

Not a big fan of the quad names, but could go either way on the others.

I will note that 2 (MSL and HLSL) use sum and product instead of add and mul so GLSL is sort of the odd one out there.

For swap directions it's a coin flip for me with a slight leaning towards shorter names.

Comment on lines +107 to +110
| `fn subgroupQuadBroadcast(e : T, id : I)` | `T` must be u32, i32, f32, f16 or a vector of those types<br>`I` must be u32 or i32 | Broadcasts `e` from the quad invocation with id equal to `id`<br>`e` must be a constant-expression<sup>2</sup> |
| `fn subgroupQuadSwapHorizontal(e : T)` | `T` must be u32, i32, f32, f16 or a vector of those types | Swaps `e` between invocations in the quad in the X direction |
| `fn subgroupQuadSwapVertical(e : T)` | `T` must be u32, i32, f32, f16 or a vector of those types | Swaps `e` between invocations in the quad in the Y direction |
| `fn subgroupQuadSwapDiagonal(e : T)` | `T` must be u32, i32, f32, f16 or a vector of those types | Swaps `e` between invocations in the quad diagnoally |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think GLSL did the wrong thing here. Why is subgroup in the name of a quad operation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Quad operations are basically clustered subgroup operations plus a guaranteed mapping of lane indices to the coordinates of the pixel grid. So it kind of makes sense, it is just overly verbose.

@Lichtso
Copy link
Copy Markdown
Author

Lichtso commented Mar 22, 2024

I am not a fan of the GLSL naming scheme either. I personally prefer the MSL naming scheme.

Anyway, from what I can tell WGSL has mostly copied the GLSL naming scheme so far.
Thus, I think there is value in sticking to it completely instead of inventing a slightly different one for people to learn and deal with.

@munrocket
Copy link
Copy Markdown
Contributor

agree with it, why subgroupSum instead of subgroupAdd if we have atomicAdd

munrocket added a commit to munrocket/gpuweb that referenced this pull request May 8, 2024
This PR is part of gpuweb#4528 which only changes two operations
```
subgroupSum -> subgroupAdd
subgroupProduct -> subgroupMul
```
And making builtin's consistent with GLSL, WGSL atomics and wgpu.
@kainino0x kainino0x added the wgsl WebGPU Shading Language Issues label May 21, 2024
kdashg pushed a commit that referenced this pull request Jun 4, 2024
This PR is part of #4528 which only changes two operations
```
subgroupSum -> subgroupAdd
subgroupProduct -> subgroupMul
```
And making builtin's consistent with GLSL, WGSL atomics and wgpu.
@kdashg
Copy link
Copy Markdown
Contributor

kdashg commented Jun 4, 2024

WGSL 2024-06-04 Minutes
  • KG: Given that we accepted Adjusts subgroup built-in name verbs to match atomics operations #4627, the only remaining change here is the one to “Quad”
  • DN: I don’t like this change. “Subgroup” seems like an overly fussy qualification, people are thinking about their Quad, not about the fact that it’s a particular kind of Subgroup operation.
  • KG: People are discouraged by long names
  • AB: Confusing to have multiple scopes in the name: subgroup and quad. Just have one.
  • DN: Renaming X and Y to Horizonal and Vertical is just too much typing - we’re in graphics, we know what our axes are
  • RESOLVED: NOT ACCEPTED

@kdashg kdashg closed this Jun 4, 2024
@kdashg
Copy link
Copy Markdown
Contributor

kdashg commented Jun 4, 2024

Thank you for the proposal, though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wgsl WebGPU Shading Language Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants