Skip to content
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

Feedback after porting a few hundred GLSL lines to WGSL #1364

Closed
baptistemanson opened this issue Jan 24, 2021 · 15 comments
Closed

Feedback after porting a few hundred GLSL lines to WGSL #1364

baptistemanson opened this issue Jan 24, 2021 · 15 comments
Labels
wgsl WebGPU Shading Language Issues

Comments

@baptistemanson
Copy link

baptistemanson commented Jan 24, 2021

Hi all,

I like to author some shaders in WebGL, on shadertoy or inside some work projects.
Here is my humble feedback after about 5h tinkering with WGSL:

  • var x : vec2<f32> = vec2<f32>(1. , 1.); gets old to type, vs vec2 x = vec2(1.,1.); for GLSL. I so often forget to put the type annotation, and current parsers are not that kind to point us towards the error.
  • Couldn't figure out how to bump the precision up. It caused a bunch of division by zero which do not fail but do garbage hard to debug
  • For buffer uniforms, I didn't find any way to expose them over except wrapped through a struct, which forced me to use more accessors.
  • I could not swizzle on function returns, like textureSample(my_vec2).x, maybe an implementation limitation?
  • Accessors such as .rgba didn't work. Also maybe an implementation limitation?
  • I didn't know how to handle const uniform expression, I usually put those outside the main in GLSL, but in WGSL it seems we put them into the main function and the compiler optimizes correctly. It's nice, but should be expressed somewhere, or documented I guess?
@baptistemanson baptistemanson added the wgsl WebGPU Shading Language Issues label Jan 24, 2021
@dj2
Copy link
Member

dj2 commented Jan 25, 2021

Which browser are you using? For the swizzling return values, and no access to rgba accessors those sound like implementation bugs. Both should work in Chrome I believe, if not, please file issues at https://bugs.chromium.org/p/tint/issues/list.

@baptistemanson
Copy link
Author

baptistemanson commented Jan 25, 2021

I used Naga, and reported those as bugs in Naga-rs as well.
There is a mention of convenience accessors in the specs, but it is so small I wasn't sure if it was not a leftover.

@litherum
Copy link
Contributor

litherum commented Jan 25, 2021

This is very good feedback, thank you!

  • var x : vec2<f32> = vec2<f32>(1. , 1.); gets old to type, vs vec2 x = vec2(1.,1.); for GLSL. I so often forget to put the type annotation, and current parsers are not that kind to point us towards the error.

This is good feedback!

See #736 for either renaming vectors to something shorter (as you suggest here), or possibly adding type aliases instead.

The group has also discussed type inference, which would turn your example into var x = vec2<f32>(1., 1.);. It's something that probably won't make the first version of WebGPU/WGSL, but it's something that's reasonable for the group to pursue in a later version.

  • Couldn't figure out how to bump the precision up. It caused a bunch of division by zero which do not fail but do garbage hard to debug

Do you think you could explain a bit more about this? (bonus points for a code snippet that caused a division by zero)

  • For buffer uniforms, I didn't find any way to expose them over except wrapped through a struct, which forced me to use more accessors.

I want to make sure I understand your desire. Did you have to have something like

struct Foo {
    myVariable: vec4<f32>;
}
[[group(1), binding(2)]] var<in> bar: Foo;

when you wanted to just write

[[group(1), binding(2)]] var<in> bar: vec4<f32>;

?

  • I could not swizzle on function returns, like textureSample(my_vec2).x, maybe an implementation limitation?

This is intended to be legal, as per the specification.

  • Accessors such as .rgba didn't work. Also maybe an implementation limitation?

Again, this is intended to be legal, as per the specification.

  • I didn't know how to handle const uniform expression, I usually put those outside the main in GLSL, but in WGSL it seems we put them into the main function and the compiler optimizes correctly. It's nice, but should be expressed somewhere, or documented I guess?

constexprs are currently under discussion, and it's not super obvious how WGSL should handle them. On one extreme, WGSL could be designed to have literally no constexprs, and all expressions would have to be be evaluated at runtime. On the other extreme, WGSL could be designed to have extremely sophisticated constexprs which support loops and function calls (like how modern C++ does). Or, WGSL could be designed to fall somewhere in the middle. It's not super obvious where the ideal spot in this spectrum would be for WGSL.

Given that, we're starting WGSL off conservatively, because it's always possible to make constexprs more flexible in the future, but it's not possible to go the other direction in the future. If I could predict what the CG will end up doing in the long term, I'd predict that we would take a reactionary approach, where we would probably make constexprs just flexible enough to address the major pain points of the community, but go no further than that.

So, if you have details about what, specifically, you wish you could write in a constexpr, that would be very helpful input into designing this feature.

@baptistemanson
Copy link
Author

baptistemanson commented Jan 25, 2021

Type inference and aliases
Type inference would help, but I can see why it's challenging to implement. I feel like vec2/vec4 are so common (we want to address sub pixel positions on extensible surfaces, they were about the only way to lookup in textures in webgl1...) that having short type aliases would be already very convenient, even sufficieent. I never used any other types of vec tbh.
Also there is the whole situation of half precision vectors that would force to change a whole file to see how it goes, but that's another matter :).

Division by zero

[[stage(fragment)]]
fn main() {
    out_target = vec4<f32>( 1.0 / 0., 0. , 0., 1.0);
}

Uniform Buffers
Exactly what you said. It's not the end of the world, just a little comment.

const Expression
I learned GLSL through Shadertoy . I never saw the color of a formal computer graphics manual.
I tend to read shaders defining a bunch of constants at the start of the shader: https://www.shadertoy.com/view/ttycWW

Beyond const expression, there is the case of dynamically uniform expressions (learned about the term today, "constant value for a given draw call"), I took the habit to move them outside the main function. I don't know if it is a good practice or not. I'm not even sure that spirv/insert your backend here would change any of its execution behavior based on the position of this computation.
Mostly, all I wanted to say is that putting a little comment on a dynamically uniform expression in an example or in the specs to say "do not worry it will will run fast even if inside the main" would have been good enough for me!

@dj2
Copy link
Member

dj2 commented Jan 25, 2021

We have aliases now, you can do type v2 = vec2<f32> if you want too. Then you can type v2 everywhere (hopefully, heh).

For constants, you can define constants and variables outside of the functions, is there something that isn't working for you?

@kvark
Copy link
Contributor

kvark commented Jan 26, 2021

Thank you for feedback!

I didn't know how to handle const uniform expression

Are you interested in knowing that from the optimization perspective? We were pondering about an idea to have a function that only accepts uniform expressions but doesn't do anything. So you'd do something like:

const myValue = <uniform_value>;
checkUniform(myValue);

As for the implementation bugs, much appreciated that you filed them! We'll sort them out :)

@kainino0x
Copy link
Contributor

The group has also discussed type inference, which would turn your example into var x = vec2<f32>(1., 1.);. It's something that probably won't make the first version of WebGPU/WGSL, but it's something that's reasonable for the group to pursue in a later version.

With #723 this does not require type inference, and I personally think it should make the cut for the first version. (At least for const, perhaps not var.)

@kainino0x
Copy link
Contributor

  • Couldn't figure out how to bump the precision up. It caused a bunch of division by zero which do not fail but do garbage hard to debug

I'm pretty sure - in theory at least - our precision is supposed to be always exactly ieee764 32-bit floats. No funny precisions like GLSL.

@kvark
Copy link
Contributor

kvark commented Jan 27, 2021

Note #674 for numerical precision. Reading this feedback and KhronosGroup/MoltenVK#1209 makes me consider this a semi-blocker for MVP.

@baptistemanson
Copy link
Author

@kvark I was interested in dynamically uniform expressions outside the main function out of habit.
I got used in JS / GLSL / Rust to remove expressions that are constant across function calls outside the function itself. For clarity and sometimes performances. Using a static block or outside the closure etc. I only lost 10min or so trying to find a syntax that would work, no biggy.

@kvark
Copy link
Contributor

kvark commented Jan 27, 2021

Thank you for clarification! I don't think we really have a separate scope for uniform data. It's not explicit, but it's observable (i.e. when trying to get a derivative for something, it has to be uniform, and we detect that). The out-of-function declarations are for private data (one per invocation, non-uniform), workgroup (uniform, but slower than registers), constant (uniform, obviously, but way more restricted than just uniform), and other stuff. Perhaps, an fn checkUniform() would be enough?

@baptistemanson
Copy link
Author

baptistemanson commented Jan 27, 2021

I'm okay with anything, even leaving dynamically uniform expressions in the main; a little comment on the example ("we can put expressions that are constant for a given render pass, the compiler optimizes it for us and computes it only once!") would be sufficient if y'all think it is having too much impact.

@litherum
Copy link
Contributor

Division by zero

[[stage(fragment)]]
fn main() {
    out_target = vec4<f32>( 1.0 / 0., 0. , 0., 1.0);
}

I think I'm missing something. How does increasing precision solve this division by zero problem? If you write a 0. literal, it will always be zero, regardless of precision.

@baptistemanson
Copy link
Author

baptistemanson commented Jan 31, 2021

I wanted to share two problems:

  • Some WGSL expression ended up being equal to 0 instead of small epsilon in the same GLSL expression with highp.
    The same expression in GLSL lowp ends up being equal to 0. So I thought it was a precision issue with WGSL, and could not find how to setup the precision - or even knowing which equivalent precision was picked for me.

Precision leading to dvision by zero
For instance, in lowp GLSL, this will be to a division by zero:

float test = 14. - 2.;
float boom = 1. / test;

Because 14. gets clamped between [-2;2]. Not in mediump or highp.

  • The expressions evaluating to 0 in WGSL and not in GLSL unearthed a second problem. When dividing by zero, computation does not fail and just return some unflushed / random memory bits (sample shared, to demonstrate that division by zero is not failing elegantly in WGSL). It looks like a security issue, where a division by zero result accesses some memory that should not be accessed.
[[stage(fragment)]]
fn main() {
    out_target = vec4<f32>( 1.0 / 0., 0. , 0., 1.0);
}

@kdashg
Copy link
Contributor

kdashg commented Sep 23, 2021

Please file new issues for the individual parts that are still bothering you!

@kdashg kdashg closed this as completed Sep 23, 2021
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this issue Sep 6, 2022
This CL adds unimplemented stubs for the `fwidthCoarse` tests.

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

No branches or pull requests

6 participants