WGSL: describe ref ptr #1569

dneto0 · 2021-03-29T18:13:07Z

- Add "memory view types" section
  - Defines reference type and pointer type
  - gives use cases and examples
  - Compares to other lanaguages
- Adds:
  - composite reference sub-expressions
  - address-of expression (unary &)
  - indirection expression (unary *)
  - variable identifier expression
  - const identifier expression
  - formal parmaeter identifier expression

TODO: grammar update for unary * and unary &

Alternate to #1368
Fixes: #1456

Builds on #1568 (rename "value types" to "plain types")

dneto0 · 2021-03-29T18:15:22Z

wgsl/index.bs

 </pre>

-## Assignment TODO ## {#assignment}
+## Assignment ## {#assignment}


Whoops, I haven't revisited this part since the original #1368 PR

github-actions · 2021-03-29T18:15:23Z

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

ben-clayton · 2021-03-29T20:55:28Z

wgsl/index.bs

+    * The left-hand side of the assignment statement must be of reference type.
+    * The right-hand side of the assignment statement must evaluate to the store type of the left-hand side.
+* The <dfn noexport>Load Rule</dfn>: Inside a function, a reference is automatically dereferenced (read from) to satisfy type rules:
+    * In a functio, when a reference expression |r| with store type |T| is used in a statement or an expression, where


wgsl/index.bs

dneto0 · 2021-03-29T22:02:12Z

I've rebased, and I think have addressed feedback from @alan-baker and @ben-clayton

alan-baker

I assume the symmetric pointer operations on . and [] will come in a subsequent PR?

wgsl/index.bs

alan-baker · 2021-03-29T23:21:53Z

wgsl/index.bs

+  <tr algorithm="assignment">
+    <td>|r| : ref<|SC|,|T|>,<br>
+        |e| : |T|,<br>
+        |T| is [=storable=],<br>


and writable?

ah. this would be "does not have access(read)"

But if it's writing to a part of a storage buffer, it's really talking about the store type of the whole buffer, and that type may include |T|.
Basically, this shows that having access attribute on type is broken. It belongs on the originating variable.
I'll add a TODO for this. Thanks!

There's a validation statement immediately after this table:

The [=originating variable=] of the left-hand side must not have an access(read) access attribute.

But it's really the store type of the originating variable....

wgsl/index.bs

dneto0 · 2021-03-30T18:58:53Z

Discussed in 2021-03-30 meeting

Approved the content. It captures the consensus so far. It's up to editors to merge when editorially ready
Discussion about the access attribute on types. This is separable from the main content of this PR, and should be raised in another issue.

kdashg · 2021-03-30T22:12:08Z

WGSL meeting minutes 2021-03-30

Describes the consensus from Have "var" as syntactic sugar for explicit pointer refs/derefs #1456
Consensus that the content is ok. For editors to merge.
AB: point about access qualifier on type, for the assignment.

kvark

This is well-written, thank you!
My main concern about this is inclusion of references in the type system. In theory, we can attempt to express the constraints in either:

the grammar - #1549, with something like an "addressable_expression" and "pointer_expression"
the type system - this PR #1569
neither - just express in words what expressions are assignable, and what can be taken a pointer of.

The downside of going with the type system is that it may not be well-suited to describe this. For example, the approach goes in a bit of conflict with "every expression has a type" rule that we wanted to preserve from a while ago. With this PR, an expression of type ref<X, Y> will go under "Load Rule" and be implicitly converted into X, based on the context...

I'd be more confident accepting this if we at least attempt to see how it would look like outside of the type system, to see if it's feasible, and what the trade-offs are going to look like.

Marking as "Request changes" so that it doesn't get accidentally merged until we properly discuss this. The PR is being rushed a little bit (not even reached the homework deadline), even if I understand the circumstances.

wgsl/index.bs

kvark · 2021-03-30T19:42:54Z

wgsl/index.bs


-Note: Pointers are not storable.
+Reference types and pointer types are both sets of memory views:
+a particular memory view is associated with a unique reference value and also a unique pointer value:


We can't compare pointer values, so does it mean anything to say "unique" here?
It's also unclear if the pointers would be unique if we could compare them, theoretically (i.e.&s.first_member == &s).

We're not talking about a comparison being done in WGSL itself, but by the (mathematical) definition of memory views.

A particular "memory view" is a mathematical object that corresponds to two WGSL values: a reference value and a pointer value. Then use duck-uniqueness: if two memory views have the same memory locations and same WGSL interpretation type, then they are the same memory views.

In fact, the colon at the end of that sentence points calls out the block-quote, which ends with " .... where p and r describe the same memory view". I think that's clear?

sorry, I don't get this still. You are saying that this talking about unique memory views, but what is written here is "unique reference value" and "unique pointer value".
Would it be possible to reword this to avoid ambiguity?

wgsl/index.bs

kvark · 2021-03-30T20:57:03Z

wgsl/index.bs

+    * The right-hand side of the assignment statement must evaluate to the store type of the left-hand side.
+* The <dfn noexport>Load Rule</dfn>: Inside a function, a reference is automatically dereferenced (read from) to satisfy type rules:
+    * In a function, when a reference expression |r| with store type |T| is used in a statement or an expression, where
+    * The only potentially matching type rules require |r| to have a value of type |T|, then


This is the scariest part of the PR, which highlights the complexity associated with trying to represent the references with the type system. Just wanted to pin it here for attention.

This is being honest about when memory is read from a variable's storage.

We need a precise statement like this so we can clearly talk about a memory model, particularly in the presence of multiple invocations executing concurrently.
Regardless of whether we support pointers or references, the spec is missing rules about what operations perform memory accesses. It needs this level of precision.

Right. As mentioned on the office hours call, it's great to have this explicitly stated in the spec.
What's not clear (and is concerning) is whether having it as a separate type (generic type) is the best way to express this, since it effectively becomes an implicit cast, the first of a kind in WGSL. And implicit casts are one of the things making the type system weaker. There are many sources that describe this, e.g. this one:

The more type coercions (implicit conversions) for built-in operators the language offers, the weaker the typing. (This could also be viewed as more built-in overloading of built-in operators.)

wgsl/index.bs

alan-baker

LGTM

dneto0 · 2021-03-31T14:11:07Z

In theory, we can attempt to express the constraints in either:

the grammar - Assignable expression #1549, with something like an "addressable_expression" and "pointer_expression"

That's a non-starter, because "identifier" is deep inside both of them. But identifiers can represent a reference value or a pointer value. You eventually are going to need the type system to kick in.
I'm arguing for being systematic about it, that's all.

- Add "memory view types" section - Defines reference type and pointer type - gives use cases and examples - Compares to other lanaguages - Adds: - composite reference sub-expressions - address-of expression (unary &) - indirection expression (unary *) - variable identifier expression - const identifier expression - formal parmaeter identifier expression TODO: grammar update for unary * and unary & Alternate to gpuweb#1368 Fixes: gpuweb#1456

@alan-baker

Addressed feedback from @alan-baker and @ben-clayton Fixed typos, examples, links, and added an example showing what happens when the expression in a return statement is a reference.

- Added the grammar rule for address-of and indirection - Fixed algorithm name text (was copypasta) - Fixed example in the assignment-statement section.

Improve cross-linking.

@kvark

Applies feedback from @kvark review. Using a complete and valid function definition allows the example to be valid WGSL. - changed the pointer-to-uniform example to pointer-to-function, because pointer-to-uniform arguments won't be valid. (Sorry, that's a restriction from baseline SPIR-V in Vulkan restriction.)

That was pulled out to gpuweb#1586

dneto0 · 2021-03-31T21:17:36Z

Rebased, fixed conflicts (with the 'let' commit that landed in the meantime). Addressed non-controverisal feedback from @kvark.

dneto0 · 2021-04-01T00:33:23Z

I peeked at C11 (draft). From what I can gather quickly:

3.14 defines 'memory location': either an object of scalar type, or a maximal sequence of adjacent bit-fields all having
nonzero width
3.15 defines 'object': region of data storage in the execution environment, the contents of which can represent
values.
3.19 defines 'value': precise meaning of the contents of an object when interpreted as having a specific type
6.2.5 starts out describing type as "The meaning of a value stored in an object or returned by a function is determined by the type of the expression used to access it." And then "Types are partitioned into object types (types that describe objects) and function types (types that describe functions). "
6.4.6 defines "An operand is an entity on which an operator acts."

Observations so far:

C memory location is really tied to language level declarations, e.g. members of a struct; adjacent bitfields are in the same "location", unless separated by a 0-width bitfield.
C 'object' is a region of storage (what WGSL would call memory locations) but is essentially polymorphic, by which I mean it could hold values of different types. (Also WGSL is more concrete, stating that a memory holds an 8bit byte.)
C 'value' is an interpretation of something stored in memory.

So far we see C has a strong orientation to storing things in memory. But for a shader language we want to have values that are never possible to store, particularly pointers. So that is a desired difference. At least, if you want pointer expressions, and all expressions to have types.

The next interesting bit is 6.5.16.1 "Simple assignment"

lists 6 different (complex-to-me) possible conditions under which "a = b" can occur. There's a number of type qualifications and "type compatible", etc.
- There's an important footnote (112): "The asymmetric appearance of these constraints with respect to type qualifiers is due to the conversion (specified in 6.3.2.1) that changes lvalues to ‘‘the value of the expression’’ and thus removes any type qualifiers that were applied to the type category of the expression (for example, it removes const but not volatile from the type int volatile * const)."
The key semantics of simple assignment are: "In simple assignment (=), the value of the right operand is converted to the type of the assignment expression and replaces the value stored in the object designated by the left
operand."

Let's parse that.

Note the "converted to the type"... Parsing the various conditions I didn't list earlier, let's focus on the "arithmetic type one":
- "the left operand has atomic, qualified, or unqualified arithmetic type, and the right has arithmetic type"
- It looks like it allows mismatch on things like const (on the RHS); also allows arithmetic type conversion (e.g. int to unsigned; int to float, etc.)
Then there's "value of the right operand". If the right operand is an "object" (e.g. because it's the name of a variable), then right there is an implicit load from memory.

The memory consistency model is 5.1.2.4 "Multi-threaded executions and data races".
E.g. "Two expression evaluations conflict if one of them modifies a memory location and the other one reads or modifies the same memory location."

I'll stop there.

kvark · 2021-04-01T17:20:30Z

@dneto0 I'm going to re-review the PR shortly, thank you for addressing my concerns!

Also, thank you for the extensive analysis of the C spec!

But for a shader language we want to have values that are never possible to store, particularly pointers. So that is a desired difference.

To be fair, we are trying to have a system where it might be possible to store pointers at some point in the future.

On the important footnote:

The asymmetric appearance of these constraints with respect to type qualifiers is due to the conversion (specified in 6.3.2.1) that changes lvalues to ‘‘the value of the expression’’ and thus removes any type qualifiers that were applied to the type category of the expression

This leads us to the idea of having "type qualifiers". A "reference" would be a qualifier on the type. It doesn't have to participate in the "main" type checking pass, but we could have a separate place where we say that:

one can only get an address of, or assign to, a type with this "reference" qualifier
there is a few things that inherit the "reference" qualifier: array indexing and member access

Sounds not very complicated to me. Am I missing anything?

I think the idea of type qualifiers is also needed for #1534, where we could say that the "access" and "stride" are the type qualifiers.

kdashg · 2021-04-07T01:10:11Z

WGSL meeting minutes 2021-04-06

DM: Main thing we’re discussing here is whether to discriminate based on type qualifiers, such as with stride annotations. We could say that this was a ‘reference qualifier’, rather than having it be within the formal type system. I think this would give us a stronger type system.
MM: Type qualifier wouldn’t be spellable? (yes) Is the qualifier equivalent to lvalue/rvalue? (not familiar with C enough to say)
RM: What’s the difference between type and type qualifier?
DM: Can bolt qualifiers on to a stable type system without changing the formal type system.
MM: Does this affect what programs can be written? (no) What type of analysis are you expecting to do on the types but not the qualifiers?
DM: Can validate more simply that the types on each side match.
DN: Can I assign 3 to 5 then?
DM: Will we need type qualifiers at all? What about access flags?
DN: I don’t think we should use type qualifiers for that.
AB: Access is a property of the memory and not the type
JG: Can't the property of memory be constrained/satisfied by the type?
AB: Not per se(?)
AB: Probably don’t want to be nesting types with different access qualifiers, ergonomically
DM: Can’t we say then that we have to trace the accessibility to the root variable?
DN: Reference itself is the set of mem locs together with how it’s interpreted
MM: The only way I could see an alternative working out here is if the sum of the complexity of the qualifier system is less than or equal to the complexity of the type system alone
DM: I think it’s more complicated to intertwine this into the type system instead of living outside of it
DM: Does Tint have a ‘real’ type for ref?
DN: Hasn’t landed yet, but yes, it’s expected to.

dneto0 · 2021-04-07T17:28:52Z

Resolution from 2021-04-06 is to let this rest for a day.

During offline discussion, the group revisited whether/how read/read-write access qualifier into the type system.
One suggestion is to embed 'read' into the pointer/ref type e.g. ptr<storage,i32,read>
We'll follow up with another issue.

Agreed to land this. (including @kvark )

This change implements pointers and references as described by the WGSL specification change in gpuweb/gpuweb#1569. reader/spirv: * Now emits address-of `&expr` and indirection `*expr` operators as needed. * As an identifier may now resolve to a pointer or reference type depending on whether the declaration is a `var`, `let` or parameter, `Function::identifier_values_` has been changed from an ID set to an ID -> Type* map. resolver: * Now correctly resolves all expressions to either a value type, reference type or pointer type. * Validates pointer / reference rules on assignment, `var` and `let` construction, and usage. * Handles the address-of and indirection operators. * No longer does any implicit loads of pointer types. * Storage class validation is still TODO (crbug.com/tint/809) writer/spirv: * Correctly handles variables and expressions of pointer and reference types, emitting OpLoads where necessary. test: * Lots of new test cases Fixed: tint:727 Change-Id: I77d3281590e35e5a3122f5b74cdeb71a6fe51f74 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/50740 Commit-Queue: Ben Clayton <[email protected]> Kokoro: Kokoro <[email protected]> Reviewed-by: David Neto <[email protected]>

dneto0 requested review from alan-baker, kvark and litherum March 29, 2021 18:13

dneto0 commented Mar 29, 2021

View reviewed changes

dneto0 added the wgsl resolved Resolved - waiting for a change to the WGSL specification label Mar 29, 2021

dneto0 added this to the MVP milestone Mar 29, 2021

ben-clayton reviewed Mar 29, 2021

View reviewed changes

dneto0 force-pushed the refptr4 branch from aac3880 to 72cb156 Compare March 29, 2021 21:13

alan-baker requested changes Mar 29, 2021

View reviewed changes

dneto0 force-pushed the refptr4 branch from 72cb156 to 5ff2109 Compare March 29, 2021 21:53

alan-baker requested changes Mar 29, 2021

View reviewed changes

dneto0 mentioned this pull request Mar 30, 2021

Add pointer-chaining expressions: ptr.field and ptr[i] #1580

Closed

kvark requested changes Mar 30, 2021

View reviewed changes

alan-baker approved these changes Mar 31, 2021

View reviewed changes

dneto0 mentioned this pull request Mar 31, 2021

Atomics proposal #1360

Closed

dneto0 added 10 commits March 31, 2021 17:14

Remove -> void from the newer examples

57d0a58

Apply review feedback

aa8d08c

Addressed feedback from @alan-baker and @ben-clayton Fixed typos, examples, links, and added an example showing what happens when the expression in a return statement is a reference.

Fix word

f4b12d9

Fix types in comments in examples

9961d71

Add grammar rule for unary &, unary *; address feedback

a839211

- Added the grammar rule for address-of and indirection - Fixed algorithm name text (was copypasta) - Fixed example in the assignment-statement section.

Fix typo in the add_one example

b0248ed

Make address-of, indirection defined terms

4ee7f51

Improve cross-linking.

Add missing access qualifier in example

e6a4db0

One more rename of "composite reference component expression"

9227867

dneto0 added 5 commits March 31, 2021 17:14

Fix caption on structure-member reference expression

23e80ab

Remove "Const identifier expression"

35db60f

That was pulled out to gpuweb#1586

For assignment statement, spell out memory *locations*

c1eee15

Add cross-reference to function-param expression

c768eba

dneto0 force-pushed the refptr4 branch from 2c6bc1c to c768eba Compare March 31, 2021 21:16

dneto0 merged commit 8268098 into gpuweb:main Apr 7, 2021

This was referenced Apr 7, 2021

consider pulling 'access' into the type of a pointer/reference #1604

Closed

Assignable expression #1549

Closed

WGSL: describe pointers #1368

Closed

arrayLength acts on poitner-to-runtime-array #1619

Merged

This was referenced Apr 20, 2021

Make ptr load/store explicit with unary *. #1455

Closed

Removing ptr and adding ref #1334

Closed

WGSL: describe ref ptr #1569

WGSL: describe ref ptr #1569

Uh oh!

Conversation

dneto0 commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dneto0 commented Mar 29, 2021

Uh oh!

alan-baker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dneto0 commented Mar 30, 2021

Uh oh!

kdashg commented Mar 30, 2021

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alan-baker left a comment

Choose a reason for hiding this comment

Uh oh!

dneto0 commented Mar 31, 2021

Uh oh!

dneto0 commented Mar 31, 2021

Uh oh!

dneto0 commented Apr 1, 2021

Uh oh!

kvark commented Apr 1, 2021

Uh oh!

kdashg commented Apr 7, 2021

Uh oh!

dneto0 commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

dneto0 commented Mar 29, 2021 •

edited

Loading

dneto0 commented Apr 7, 2021 •

edited

Loading