-
Notifications
You must be signed in to change notification settings - Fork 353
wsgl: Add rule that offsets must be compile time constant #1238
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
Conversation
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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to state:
offsetmust be compile time constant, and may only be provided as a module constant, literal orconst_exprexpression (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.
|
Discussed at the 2020-11-30 meeting. |
…constant We really want a new `constexpr`.
|
Created #1272 to discuss |
| `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> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
* 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`.
) * 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]>
This CL adds unimplemented stubs for the `smoothstep` builtin. Issue: gpuweb#1238
Issue: #1235