Skip to content

Assignable expression #1549

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.

Sign up for GitHub

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

Jump to bottom
Closed
kvark wants to merge 1 commit into from
Closed

Assignable expression #1549

kvark wants to merge 1 commit into from

Conversation

@kvark
Copy link
Contributor

@kvark kvark commented Mar 22, 2021

This is a follow-up to #1456, on the topic of the type system reach with regards to references. This mini-proposal is developed in collaboration with the white wolf @jimblandy.

Basically, we all want WGSL to be able to statically check that the left side of the assignment statement is assignable. The disagreement is about what part of a static check is responsible for this. We can consider at least 3 different places for this:

  1. grammar
  2. type system
  3. post-type checks (like Rust's borrow checker, or other validation rules in WGSL)

This PR shows how we can get the static check with (1) + (3): grammar would ensure specific form of the left-hand expression, and the validation rules would check for mutability (i.e. is the IDENT really a const or a uniform buffer?). The proposal excludes pointers (since the pointers spec still needs to land either way, it's incomplete right now). I think it looks very simple and approachable.

The benefit of going this route is that the type system would not need to have any implicit coersions (from references to values on the right hand side).

@kvark kvark added the wgsl WebGPU Shading Language Issues label Mar 22, 2021
@kvark kvark requested a review from dneto0 March 22, 2021 20:08
@github-actions
Copy link
Contributor

github-actions bot commented Mar 22, 2021

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

dneto0
dneto0 reviewed Mar 22, 2021
View reviewed changes
wgsl/index.bs
Otherwise, singular expression is a pointer expression in an Assigning (L-value) context
which maps to OpAccessChain followed by OpStore

assignment_statement
Copy link
Contributor

@dneto0 dneto0 Mar 22, 2021

Choose a reason for hiding this comment

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

There is already an assignment_statement production

Does this PR mean to add to it, or replace it?

Copy link
Contributor Author

@kvark kvark Mar 23, 2021

Choose a reason for hiding this comment

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

Github diff showing it in a weird way, but hey, it's just 6 lines!
The PR only changes the assignment_statement, doesn't add a new one

Copy link
Contributor

@dneto0 dneto0 Mar 24, 2021

Choose a reason for hiding this comment

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

Ah, I see that.
I was tripped up by the fact that you've replaced singular_expression with IDENT (postfix_expression)*

That's a regression in the grammar that prevents implementation of the ptr/ref resolutions from #1456 and #1530.

@dneto0
Copy link
Contributor

dneto0 commented Mar 22, 2021

I don't think this approach scales.
The "right" way to do it is via the type system.

The assignment statement is a store of a value to storage in memory. The thing on the RHS is the thing you store. It has a type (which must be "storable", as defined in the spec). The thing on the LHS is a reference to storage with a particular store type. The store type of the LHS must be the same as the type on the RHS.
So type checking is going to get involved somehow. To say you can avoid the type system on the LHS implies that you can equally avoid the type system on the RHS. That makes little sense to me. You need more than grammar; you need the types too.

The cleanest way is to define types that denote a view into locations in a particular storage class, such that you can store a value of a particular store type.
Right now the spec only has "ptr" as the one and only kind of "view". A "ref" will be the other way.
The main reason to have the split between "ptr" and "ref" is because of the insistence that when you make an alias to the storage in part of a variable, the committee said they definitely wanted to see an explicit marker to indicate it's an alias.
So, for call to a function with a reference parameter you want to see something like modify_it(&myvar), not just plain modifyit(myvar).
And when you store through the alias, you want to see explicit mention, so the desire to see corresponding "*" as in *ptr = foo.

kvark
kvark commented Mar 23, 2021
View reviewed changes
Copy link
Contributor Author

@kvark kvark left a comment

Choose a reason for hiding this comment

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

I don't think this approach scales.

Could you elaborate please? What does it need to scale to?

The store type of the LHS must be the same as the type on the RHS.
So type checking is going to get involved somehow.

Yes, precisely in the way you described. We'd just say that the type on the left has to match the type on the right.
Separately, we'd say that the IDENT on the LHS has to be pointing to a store-able variable.

This seems much easier than to go into the whole ref territory with implicit coersions.

wgsl/index.bs
Otherwise, singular expression is a pointer expression in an Assigning (L-value) context
which maps to OpAccessChain followed by OpStore

assignment_statement
Copy link
Contributor Author

@kvark kvark Mar 23, 2021

Choose a reason for hiding this comment

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

Github diff showing it in a weird way, but hey, it's just 6 lines!
The PR only changes the assignment_statement, doesn't add a new one

dneto0
dneto0 reviewed Mar 24, 2021
View reviewed changes
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.

This blocks progress on other consensus items.

wgsl/index.bs
Otherwise, singular expression is a pointer expression in an Assigning (L-value) context
which maps to OpAccessChain followed by OpStore

assignment_statement
Copy link
Contributor

@dneto0 dneto0 Mar 24, 2021

Choose a reason for hiding this comment

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

Ah, I see that.
I was tripped up by the fact that you've replaced singular_expression with IDENT (postfix_expression)*

That's a regression in the grammar that prevents implementation of the ptr/ref resolutions from #1456 and #1530.

@kvark
Copy link
Contributor Author

kvark commented Mar 24, 2021

@dneto0 could you clarify on how this is blocking?

@dneto0 dneto0 mentioned this pull request Mar 31, 2021
Merged
@dneto0
Copy link
Contributor

dneto0 commented Apr 7, 2021

#1569 has landed. Suggest we close this.

@kvark kvark closed this Apr 7, 2021
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
@shrekshao
validation/render_pipeline/inter_stage.spec.ts (gpuweb#1549)
4fd8cfe 
* Inter stage validations

* max components fix

* max shader variable location and fixes

* fix

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

Reviewers

@dneto0 dneto0 dneto0 left review comments

Assignees

No one assigned

Labels

wgsl WebGPU Shading Language Issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kvark @dneto0