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

Add workgroupuniform load built-in and analysis #3586

Merged
merged 3 commits into from
Dec 5, 2022

Conversation

alan-baker
Copy link
Contributor

@alan-baker alan-baker commented Nov 9, 2022

Fixes #2586

  • Add workgroupUniformLoad built-in function as
    new synchronization built-ins
  • Rework synchronization built-ins to match style of other sections
  • Modify rvalue expression table to split variable identifier rows into
    two
    • the first for when the load rule is not invoked maintains the
      incoming uniformity (that is pointers and references are uniform
      while they remain memory views)
    • the second for when the load rule is invoked retains the previous
      behaviour

@alan-baker alan-baker added the wgsl WebGPU Shading Language Issues label Nov 9, 2022
@alan-baker alan-baker added this to the V1.0 milestone Nov 9, 2022
@alan-baker alan-baker added the uniformity Issues / discussions around uniformity analysis label Nov 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Previews, as seen when this build job started (6198f55):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
@alan-baker
Copy link
Contributor Author

Open question: do we want a version of workgroupBroadcast based on local invocation index?

wgsl/index.bs Outdated Show resolved Hide resolved
@dneto0
Copy link
Contributor

dneto0 commented Nov 16, 2022

From the 2022-11-15 meeting, the committee agreed to proceed with workgroupUniformLoad.

wgsl/index.bs Show resolved Hide resolved
@dneto0 dneto0 mentioned this pull request Nov 17, 2022
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

workgroupBroadcast is not yet agreed by the committee.

workgroupUniformLoad needs further qualification: The result is uniform only if the pointer argument is uniform. Details posted to the issue.

wgsl/index.bs Outdated
space.
</table>

### `workgroupBroadcast` ### {#workgroupBroadcast-builtin}
Copy link
Contributor

Choose a reason for hiding this comment

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

Committee 2022-11-15 did not (yet) agree to workgroupBroadcast

dneto0 pushed a commit to dneto0/gpuweb that referenced this pull request Nov 18, 2022
Make the synchronzation builtins section look like the other
builtins sections.

This takes the editorial-only changes from gpuweb#3586
Fixes gpuweb#2586

* Add workgroupUniformLoad built-in function as
  new synchronization built-ins
* Rework synchronization built-ins to match style of other sections
* Modify rvalue expression table to split variable identifier rows into
  two
  * the first for when the load rule is not invoked maintains the
    incoming uniformity (that is pointers and references are uniform
    while they remain memory views)
  * the second for when the load rule is invoked retains the previous
    behaviour
@alan-baker alan-baker changed the title Add workgroup broadcast and uniform load built-ins Add workgroupuniform load built-in and analysis Nov 29, 2022
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

Thanks!

wgsl/index.bs Outdated
@@ -9805,6 +9807,13 @@ The rules for analyzing expressions take as argument both the expression itself
<td class="nowrap">*CF*, *CF*
<td>
<tr><td>identifier [=resolves|resolving=] to function-scope variable "x"
where the [=load rule=] is not invoked during [=type checking=]
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep.
This aligns uniformity analysis with how we do type-checking and the conceptual model of how execution/evaluation occurs.
This is the end result of what @jimblandy suggested, and it works out really nicely.

@dneto0
Copy link
Contributor

dneto0 commented Nov 29, 2022

Would also fix #3602

* Move function scope pointer desugaring to a higher level section
* generalize to pointer desugaring
  * function-scope pointer parameter variable substitution
  * let-declaration desugaring
* update modified identifier expression rules to require that "x" is
  also the root identifier
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

I think there's a technical problem with the pointer desugaring, due to incomplete capture of values of variables that may change between the pointer declaration and its use.

wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated
Each [=let-declaration=] with an [=effective-value-type=] that is a [=pointer
type=] is desugared as follows:
* The initializer expression of the declaration is recorded.
* Each [=identifier=] that [=resolves=] is substituted with the recorded
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing words here? Each identifier that resolves to a let-declaration ...

Interestingly, it doesn't matter what order the substitutions are made.

Might want to add a note:

The order of substitutions is not significant, always settling with the same final result.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a technical problem here? Because the indexing into those desugared pointers could have its uniformity properties changed. They may refer to variables that have had full assignments on them in the meantime, or vice versa. Basically, you have to properly capture the values of variables at the point of recording the initializer for the let-decls.

E.g. see how 'foo' evolves through desugaring. Yet, variable i receives a full assignment that makes it uniform halfway through the function.

@group(0) @binding(0) var t: texture_2d<f32>;
@group(0) @binding(1) var s: sampler;

fn foo(parami:i32) -> vec4<f32> {
   var i = parami;
   var v = array<i32,2>(-1,1);
   let p = &v[i];
   if (v[i] > 0) {
      return textureSample(t,s,vec2(.0));  // causes uniformity error.
   }
   i = 0;   // full assignment
   if (v[i] > 0) {
      return textureSample(t,s,vec2(.0)); // no uniformity error
   }
   return vec4<f32>();
}

fn foo_via_pointers(parami:i32) -> vec4<f32> {
   var i = parami;
   var v = array<i32,2>(-1,1);
   let p = &v[i];
   if (*p > 0) {
      return textureSample(t,s,vec2(.0));  // should cause uniformity error.
   }
   i = 0;  // full assignment
   if (*p > 0) {
      return textureSample(t,s,vec2(.0)); // should still cause uniformity error
   }
   return vec4<f32>();
}

fn foo_via_pointers_desugared(parami:i32) -> vec4<f32> {
   var i = parami;
   var v = array<i32,2>(-1,1);
   let p = &v[i];
   if (*(&v[i]) > 0) {
      return textureSample(t,s,vec2(.0));  // should cause uniformity error.
   }
   i = 0;  // full assignment
   if (*(&v[i]) > 0) {
      return textureSample(t,s,vec2(.0)); // should still cause uniformity error
                                          // but it won't
   }
   return vec4<f32>();
}

@fragment
fn main(@builtin(position) p: vec4<f32>) -> @location(0) vec4<f32> {
  return foo(i32(trunc(p.x)));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. The desugaring should be capturing the values of mutable variables other than the root identifier.

wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
* Capture component subexpression values when desugaring pointers
* fix references and typos
* Added a note about type checking ordering
* Improve wording of identifier expression uniformity rules
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks for handling this delicate rule!

* Each [=identifier=] that [=resolves=] is substituted with the recorded
initializer expression (wrapped in a
[[#parenthesized-expressions|parenthesized expression]]).
* Visit each subexpression, *SE*, of the initializer expression of *L* in a postorder depth-first traversal:
Copy link
Contributor

Choose a reason for hiding this comment

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

Right. It has to be post-order because you want to capture the most deeply nested item first.

Then I started thinking: Does this have to specify left-to-right traversal as well? I don't think it does.
Interestingly, the desugaring doesn't have to produce and equivalent value, just an equivalent uniformity property.
And the only mutable variable that can have its uniformity affected is a function-scope var. And the analysis only updates that uniformity property in an assigment statement in the same function. So this desugaring is sufficient.

If we actually wanted to capture the right value, we'd also have to capture the values and side effects of function calls. That's a lot more work, and not needed here.

the [=root identifier=] of the [=memory view=]
<tr><td>identifier [=resolves|resolving=] to function-scope variable "x",
where the identifier appears as the [=root identifier=] of a [=memory view=]
expression, *MVE*, and the [=load rule=] is not invoked on *MVE* during
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Might be worth emphasizing the "not", either italics or bold.

Copy link
Contributor

Choose a reason for hiding this comment

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

An example suggested from internal review:

Consider let p = &y[x[10]];

In this case:

  • one MVE is x[10] and it does have the load rule applied. The root identifier is x.
  • the other MVEs are y[x[10]] and &y[x[10]], with root identifier y, but neither of those have the load rule applied during type checking.

Copy link
Contributor

@jrprice jrprice left a comment

Choose a reason for hiding this comment

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

The mechanics of this LGTM, and aligns with how we plan to implement this in Tint.

We can also remove the special-case for arrayLength from the analysis, but happy for that to be a follow-up change if preferred.

@dneto0
Copy link
Contributor

dneto0 commented Dec 5, 2022

Discussed internally at Google:

  • This can be expanded in future when more general pointers are added.
    • In particular, when adding pointer-to-workspace params, they can be desugared into fresh module-scope variables in workgroup address space; where "fresh" means each pointer formal param in the source corresponds to a distinct module-scope variable. That's sufficient for analysis because we assume the aliasing restrictions are still in place. The variable would be seen as non-uniform because it's still mutable and shared among invocations. There may be other ways to handle this too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
uniformity Issues / discussions around uniformity analysis wgsl WebGPU Shading Language Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workgroup Broadcast
4 participants