Skip to content

Conversation

@kvark
Copy link
Contributor

@kvark kvark commented Mar 17, 2021

Helps #736 and #560 to the point of being able to postpone the rest till after MVP.

When porting shaders to WGSL, writing down the type for every constant appears to be the most painful thing. Consider this shader for example, here is a slice of it:

    vec4 a0 = b0.xzyw + s0.xzyw*sh.xxyy;
    vec4 a1 = b1.xzyw + s1.xzyw*sh.zzww;
    vec3 p0 = vec3(a0.xy, h.x);
    vec3 p1 = vec3(a0.zw, h.y);
    vec3 p2 = vec3(a1.xy, h.z);
    vec3 p3 = vec3(a1.zw, h.w);

Writing all of these with types is just painful:

    const a0: vec4<f32> = b0.xzyw + s0.xzyw*sh.xxyy;
    const a1: vec4<f32> = b1.xzyw + s1.xzyw*sh.zzww;
    const p0: vec3<f32> = vec3<f32>(a0.xy, h.x);
    const p1: vec3<f32> = vec3<f32>(a0.zw, h.y);
    const p2: vec3<f32> = vec3<f32>(a1.xy, h.z);
    const p3: vec3<f32> = vec3<f32>(a1.zw, h.w);

I generally want the language to be minimal and oppose the minor ergonomic changes if they increase complexity. This problem though is a major blocker for WGSL adoption in my eyes. Types are verbose and rich in special symbols (:, <, >), and most importantly - they are redundant in all cases where we can infer them. I believe our API is currently structured in a way that makes the right-hand-size expression always have a defined type, so inferring the const declarations is trivial.

Justification to limiting this change to const only:

  • const should be preferred over var. Making the former easy to write digs a good pit of success for WGSL.
  • just limiting the PR to the minimal scope where I believe it solves the problem.

Edit: this is what it will look like:

    const a0 = b0.xzyw + s0.xzyw*sh.xxyy;
    const a1 = b1.xzyw + s1.xzyw*sh.zzww;
    const p0 = vec3<f32>(a0.xy, h.x);
    const p1 = vec3<f32>(a0.zw, h.y);
    const p2 = vec3<f32>(a1.xy, h.z);
    const p3 = vec3<f32>(a1.zw, h.w);

@kvark kvark requested a review from dneto0 March 17, 2021 16:27
@RobinMorisset
Copy link
Contributor

Myles and I think that this could be helpfully extended to vars, but we are 100% in favor of doing it for const even if the group decides not to do it for vars.

@alan-baker alan-baker added the wgsl WebGPU Shading Language Issues label Mar 18, 2021
@dneto0
Copy link
Contributor

dneto0 commented Mar 18, 2021

I agree that every expression in a valid WGSL program must have a single type, and that can be inferred bottom-up (#723).

I think we can go one step further. In #1289 (post-MVP mileston) I argued for even more abbreviated syntax.

So instead of

const a0 = b0.xzyw + s0.xzyw*sh.xxyy;
const a1 = b1.xzyw + s1.xzyw*sh.zzww;
const p0 = vec3<f32>(a0.xy, h.x);
const p1 = vec3<f32>(a0.zw, h.y);
const p2 = vec3<f32>(a1.xy, h.z);
const p3 = vec3<f32>(a1.zw, h.w);

We would have:

a0 := b0.xzyw + s0.xzyw*sh.xxyy;
a1 := b1.xzyw + s1.xzyw*sh.zzww;
p0 := vec3<f32>(a0.xy, h.x);
p1 := vec3<f32>(a0.zw, h.y);
p2 := vec3<f32>(a1.xy, h.z);
p3 := vec3<f32>(a1.zw, h.w);

That's even fewer characters to type on your keyboard. It also makes using 'const' the path of absolutely minimal resistance, and that's good.

It turns out I had seen this before, from Go. See https://golang.org/ref/spec#Short_variable_declarations
The differences are:

  • In Go, it's used to declare mutable variables, rather than const
  • In Go, you can have a list of variables on the left, as a destructuring assigmment. That's a good idea, but how about for post-MVP.

The ugliest part of my #1289 proposal was dealing with pointer vs. non-pointer. But since then we've chosen a direction where we have split (dual) types for "ref" and "ptr", and so there's no longer a confusion.

So we can cleanly do both : declare a pointer and declare a copied value:

       var age : i32 = 12;
       p := &age;        // same as      const p : ptr<function,i32> = &age;
       copyage := age;   // same as      const copyage : i32 = age; 

A clear followup is to infer the store type for a variable as well. It uses the same principle. Two possible syntaxes suggest themselves:

       var age = 12;  

or

       var age := 12;     //  It looks like we just removed the type from the standard form of the declaration.

@kvark
Copy link
Contributor Author

kvark commented Mar 19, 2021

@dneto0 I'm tempted to say "let's discuss := as a follow-up" since it blows out the scope of this PR and asks for much more time discussing the proper syntax. Just refining const is an easier thing to buy into right now.

In your (future?) proposal:

  • would it replace const completely inside function bodies?
  • if so, would you be able to specify the type if you want? something like `p : i32 = 1"
  • wouldn't this complicate the parser? Right now, if it sees an identifier, it at least expects it to be known (i.e. assignment or function call). With your syntax, it's going to need more logic

@dneto0
Copy link
Contributor

dneto0 commented Mar 23, 2021

Discussed at 2021-03-23 meeting

@kvark
Copy link
Contributor Author

kvark commented Mar 23, 2021

Agreed on the call to land this and follow-up with type inference for variables.

@kvark
Copy link
Contributor Author

kvark commented Mar 23, 2021

@dneto0 it's rebased now. Any concerns about the wording?

@github-actions
Copy link
Contributor

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
51088f5

@kvark kvark requested a review from dneto0 March 23, 2021 19:17
@dneto0
Copy link
Contributor

dneto0 commented Mar 23, 2021

Answering:

would it replace const completely inside function bodies?

It's important to allow the programmer to be very explicit. I would allow both:

    const a: i32 = 12;
    another := 12;
  • if so, would you be able to specify the type if you want? something like `p : i32 = 1"

I would not allow that. If you want to be explicit, write the whole thing: const p : i32 = 1;

  • wouldn't this complicate the parser? Right now, if it sees an identifier, it at least expects it to be known (i.e. assignment or function call). With your syntax, it's going to need more logic

No. It does not complicate the parser. The parsing into cases of call_statement, assignment_statement before checking for known-ness of the identifier. Adding my proposed case adds a "short_const_decl" case. It adds one more token into the follow-set of identifier: '(' for call expression, '=' for assignment, and adds ':=' for short-const-decl.

: variable_decl
| variable_decl EQUAL short_circuit_or_expression
| CONST variable_ident_decl EQUAL short_circuit_or_expression
| CONST (IDENT | variable_ident_decl) EQUAL short_circuit_or_expression
Copy link
Contributor

Choose a reason for hiding this comment

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

The 'variable_ident_decl' production also starts with IDENT.

I think there are already other shortcuts like this in the grammar, so we can revisit that if/when we get a mechanically-processable grammar.

@dneto0 dneto0 merged commit 405014d into gpuweb:main Mar 24, 2021
@kvark kvark deleted the type-inference branch March 24, 2021 14:52
@jespertheend
Copy link
Contributor

Does this mean types from functions will be inferred as well?
ie:

fn myFunction -> vec2<f32> {
    return vec2<f32>(1.0, 1.0);
}

const a = myFunction();

@RobinMorisset
Copy link
Contributor

Does this mean types from functions will be inferred as well?
ie:

fn myFunction -> vec2<f32> {
    return vec2<f32>(1.0, 1.0);
}

const a = myFunction();

I don't see why not. The inference is still purely local: myFunction() will always return vec2, and it is declared to do so.

@kainino0x
Copy link
Contributor

It's slightly more complicated because it's inferred from multiple expressions. I think it has less value and isn't very worth it.

@jespertheend
Copy link
Contributor

Also now that I think about it, it could potentially make the code less readable.
const a = myFunction(); doesn't really show what the type of a is, you'd have to go to the function definition in order to find out.

@kvark
Copy link
Contributor Author

kvark commented Mar 30, 2021

Yes, there was no intention to infer function types. Let's not go into this territory, please :)

@munrocket
Copy link
Contributor

And what about trigonometric functions, texture samples and builtIn variables?

var angle : f32 = -atan2(a_particleVel.x, a_particleVel.y);
var uv : vec2<f32> = floor(30.0 * fragUV);
var texColor : vec4<f32> = textureSample(myTexture, mySampler, fragUV * 0.8 + vec2<f32>(0.1, 0.1));
var dims : vec2<i32> = textureDimensions(inputTex, 0);
var index : u32 = GlobalInvocationID.x;

Is there any clear rule when we should stop infer types? Because this thing works differently in Firefox/Chrome currently. https://github.com/austinEng/webgpu-samples/pull/121

@kdashg
Copy link
Contributor

kdashg commented Jun 29, 2021

Please do not get caught up in the status of prototype implementations just yet. We will be enforcing conformance via testing.

@kainino0x
Copy link
Contributor

It's slightly more complicated because it's inferred from multiple expressions. I think it has less value and isn't very worth it.

Hm, I think when I said this I thought the suggestion was to infer the return type of functions. That we shouldn't do.
However, inferring the type of a var or const like const x = myFunction(); is consistent with the other rules, and it is already supported in the spec. I think local var and const declarations (including all of your examples @munrocket) never require type annotations.

ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
This PR updates existing storage texture format tests to
expect exceptions rather than validation errors in those
cases.
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
The changes to check for exceptions being thrown in gpuweb#1529 should
only apply to cases where we are testing for an required feature
being enabled. The updates to createBindGroupLayout.spec.ts
were unnecessary and have started causing failures.

FYI @Gyuyoung

Issue: gpuweb#919
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.

8 participants