Skip to content

Conversation

@ben-clayton
Copy link
Contributor

Issue: #1235

@dj2 dj2 added the wgsl WebGPU Shading Language Issues label Nov 19, 2020
@dj2 dj2 added this to the MVP milestone Nov 19, 2020
@ben-clayton ben-clayton marked this pull request as ready for review November 19, 2020 17:35
wgsl/index.bs Outdated
texture wrapping modes. <br>
`offset` must be compile time constant, and may only be provided as a
[literal](#literals), [constant expression](#module-constants) or from a
[const](#variables) variable. <br>
Copy link
Contributor

Choose a reason for hiding this comment

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

A 'const' declaration is re-evaluated each time it is encountered in program order. So if we want to allow this, it should be a module-scope const declaration

... or from a module-scope const value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copying discussion from private chat:

@ben-clayton :

So a wgsl function scoped const is just an immutable variable (much like C++), while a module-scoped one is compile time constant (i.e. a SPIR-V OpConstant)?
Is that right?


@dneto0 :

Yes.
The main difference from C++ is that in C++ you can take the address of a const, but in WGSL you won't be able to.


I find it rather odd that const means compile-time-constant when at module scope and read-only when at function scope, and I suspect this will be rather confusing for developers.

Maybe adding a real constexpr to the language is preferable here?

Perhaps while we're debating this I should remove the reference to variables, requiring the offset to be a literal or "vector literal" (for want of a better term)?

Copy link
Member

Choose a reason for hiding this comment

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

Note, that's not what we do in Tint when converting to SPIR-V. We look at the function const constructor and determine if it's compile time const and, if so, turn it into a real compile time const instead of a var. The const becomes a read-only var if the initialization expression needs to be evaluated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it rather odd that const means compile-time-constant when at module scope and read-only when at function scope, and I suspect this will be rather confusing for developers.

I didn't think it was confusing. E.g. it's just like in the following idiomatic C fragment:

   for ( int i = 0; i < n ; i++ ) {
       // The 'twice' identifier references an unchanging value for its whole lifetime. 
       // But it's different on each iteration.
       const int twice = 2 * i;  

       foo(twice)
   }

Copy link
Member

@dj2 dj2 Nov 19, 2020

Choose a reason for hiding this comment

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

In Tint at the moment, if you had:

   for ( var i : i32 = 0; i < 2 ; i++ ) {
       const twice : i32 = 2;  

       foo(twice * i)
   }

the twice would be an OpConstant. But in the case const twice : i32 = 2 * i it would be an OpVariable that gets set each iteration of the loop. At module scope the initializer can only be const_expr so it's always some kind of Constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to state:

offset must be compile time constant, and may only be provided as a module constant, literal or const_expr expression (e.g. vec2<i32>(1, 2)).

This does not include const at function scope, which I personally feel isn't well defined enough to include*. I'd love to see a constexpr variable type to address this, but we can discuss this later.


*My concerns are: if we state here that a function const can be treated as constexpr if the RHS is a const_expr, then does this propagate?

So if this is legal:

const offset : vec2<i32> = vec2<i32>(1, 2);
textureSample(t, s, c, offset);

Then I assume this is:

const x : i32 = 1;
const y : i32 = 2;
textureSample(t, s, c, vec2<i32>(x, y));

But what about:

const xy : vec2<i32> = vec2<i32>(x, y);
const offset : vec2<i32> = xy;
textureSample(t, s, c, offset);

Or even:

const x : i32 = 1;
const y : i32 = 2;
const z : i32 = 2;
const xy : vec2<i32> = vec2<i32>(x, y);
const offset : vec3<i32> = vec3<i32>(xy, z);
textureSample(t, s, c, offset);

If this were legal, I think this is too close to implicit magic for me - you can't tell whether a variable is allowed to be used as an offset just by looking at the variable declaration. That's why I'd be keen on constexpr.

@grorg
Copy link
Contributor

grorg commented Nov 30, 2020

Discussed at the 2020-11-30 meeting.

…constant

We really want a new `constexpr`.
@ben-clayton
Copy link
Contributor Author

Created #1272 to discuss constexpr.

`offset` must be compile time constant, and may only be provided as a
[module constant](#module-constants), [literal](#literals) or
`const_expr` expression (e.g. `vec2<i32>(1, 2)`).<br>
[literal](#literals) or `const_expr` expression (e.g. `vec2<i32>(1, 2)`).<br>
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't literals a subset of const_expr expressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I was trying to express this in a way that doesn't involve the reader having to understand the terminology of our grammar. I can remove it if it offends.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, sounds like something the editors could resolve for us :)

@kdashg kdashg merged commit 051459b into gpuweb:main Jan 19, 2021
@kdashg
Copy link
Contributor

kdashg commented Jan 19, 2021

@ben-clayton ben-clayton deleted the tex-offset-constexpr branch January 19, 2021 19:37
kvark pushed a commit that referenced this pull request Jan 19, 2021
* wsgl: Add rule that offsets must be compile time constant

* wsgl: Address review comments (1)

* wsgl: Remove the ability to specify `offset` arguments with a module constant

We really want a new `constexpr`.
kdashg pushed a commit that referenced this pull request Jan 26, 2021
)

* wsgl: Add rule that offsets must be compile time constant

* wsgl: Address review comments (1)

* wsgl: Remove the ability to specify `offset` arguments with a module constant

We really want a new `constexpr`.

Co-authored-by: Ben Clayton <[email protected]>
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
This CL adds unimplemented stubs for the `smoothstep` builtin.

Issue: gpuweb#1238
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
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.

6 participants