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

Use explicit "vulkan-style" shaders to generate all shader outputs #333

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cart
Copy link

@cart cart commented May 9, 2020

In order to create a WebGPU backend, we need shaders with separate sampler and texture2D types. Unfortunately there is no existing automated way (that I know of) to convert a sampler2D shader into a split sampler and texture2D shader.

I can think of two solutions to this problem:

  1. create duplicate "vulkan style" shaders with separate samplers and textures
  2. Make the source shaders "vulkan style" with separate samplers and textures, generate spirv from those, then generate compatible shaders for each backend using spirv-cross

(1) seems harder to maintain so I opted to implement (2).

The changes in this pr:

  • Adapt source shaders to a vulkan style format with split samplers and textures.
  • Modify shader makefile to generate gl3, gl4, and metal shaders from the source shaders by using spirv-cross
  • Modify GL backend to account for format changes:
    • Fall back to a SPIRV_Cross_Combined sampler2D uniform name if a "normal" uniform name cannot be found
    • Use vec4 types instead of scalar, matrix, vec2, and vec3 types when binding uniforms. This change is the result of the spirv-cross transformation. I would really prefer to not make this change, but I couldn't find a way to force spirv-cross to maintain the original types.

I tested the GL backend and it works as expected.

This PR is not ready to be merged for the following reasons:

  • We should discuss the above vec4 change in the GL backend and generated shaders
  • Need to restore #ifdef GL_ES precision highp sampler2D in GL outputs. The ifdef is still in the source shaders, but the initial spirv compilation strips that out. This could be fixed by creating a separate "gles" shader resource folder generated with -DGL_ES. Alternatively we could do a text transformation on the existing GL outputs.
  • #extension GL_GOOGLE_include_directive : enable is also stripped out. This should also be added to the GL outputs
  • Haven't tested on android
  • Haven't tested WebGL backend
  • Metal backend isn't functional yet. The metal shaders generated appear valid, but I think the backend can't bind uniforms because the argument names have changed:

Original clear.vs.metal

// Automatically generated from files in pathfinder/shaders/. Do not edit!
#include <metal_stdlib>
#include <simd/simd.h>

using namespace metal;

struct main0_out
{
    float4 oFragColor [[color(0)]];
};

fragment main0_out main0(constant float4& uColor [[buffer(0)]])
{
    main0_out out = {};
    out.oFragColor = float4(uColor.xyz, 1.0) * uColor.w;
    return out;
}

New clear.vs.metal

Note the new uColor struct and the argument name changing from uColor to `_12

// Automatically generated from files in pathfinder/shaders/. Do not edit!
#include <metal_stdlib>
#include <simd/simd.h>

using namespace metal;

struct uColor
{
    float4 color;
};

struct main0_out
{
    float4 oFragColor [[color(0)]];
};

fragment main0_out main0(constant uColor& _12 [[buffer(0)]])
{
    main0_out out = {};
    out.oFragColor = float4(_12.color.xyz, 1.0) * _12.color.w;
    return out;
}

This issue seems solvable by adjusting the metal backend to query uniforms using argument type names instead of argument names.

Once again I'm pulling in @kvark and @tangmi because this relates to WebGPU / wgpu.

@cart
Copy link
Author

cart commented May 9, 2020

Just like #332, there are tradeoffs here ... so i'd love to hear people's thoughts.

@kvark
Copy link
Member

kvark commented May 10, 2020

This issue seems solvable by adjusting the metal backend to query uniforms using argument type names instead of argument names.

I don't understand this part. In gfx/wgpu world, there is no querying from a backend. You describe the pipeline layout, and the backend uses it for shaders. In case of wgpu, it also validates the shader needs to be compatible with the layout. Names don't mean anything for these.

I can think of two solutions to this problem:

Any reason to ignore the suggestion I voiced on Matrix?

#if COMBINED_IMAGE_SAMPLERS
  uniform sampler2D texFoo;
  uniform sampler2D texBar;
#else
  layout(set=0, binding=0) uniform sampler;
  layout(set=0, binding=1) uniform texture2D texFoo_;
  layout(set=0, binding=2) uniform texture2D texBar_;
  #define texFoo sampler2D(texFoo_, sampler)
  #define texBar sampler2D(texBar_, sampler)
#endif

This would mean all the code keeps using sampler2D things like it does now. Only the declaration portions change between wgpu and GL.

@cart
Copy link
Author

cart commented May 10, 2020

I don't understand this part. In gfx/wgpu world, there is no querying from a backend. You describe the pipeline layout, and the backend uses it for shaders. In case of wgpu, it also validates the shader needs to be compatible with the layout. Names don't mean anything for these.

This is specifically the "pathfinder metal backend", which needs to map a "uniform name string" to a MetalUniformIndex in order to bind a value. It currently uses reflection on the main function's argument names to find the index. Check out MetalDevice::get_uniform_index for context.

Any reason to ignore the suggestion I voiced on Matrix?

This is embarrassing for me, but I convinced myself it didn't work. The example you gave me didn't compile with glslValidator and i tried to adapt it without success. But with a few changes I was able to make it work:

// ifdef instead of if
#ifdef COMBINED_IMAGE_SAMPLERS
uniform sampler2D uSrc;
#else
// "sampler" by itself didnt work for me. i needed to give it a name
layout(set=0, binding=0) uniform sampler uSampler;
layout(set=0, binding=1) uniform texture2D uSrc_;
#define uSrc sampler2D(uSrc_, uSampler)
#endif

I gave up too early the first time I tried and I just started diving into other solutions. I should have circled back with you when I couldn't get it to work.

Given that this works ... its clearly the right solution. I'll put together a new branch and I'll reach out if/when everything works.

Sorry for wasting your time!

@cart
Copy link
Author

cart commented May 10, 2020

I just remembered why I moved on so quickly (and it wasn't just the compilation issues). The #ifdef above works in isolation. You can compile a gl 330 shader using glslangValidator -G330 -DCOMBINED_IMAGE_SAMPLERS and a "vulkan" shader using glslangValidator -V, but how do we make a more complex example work?

#ifdef COMBINED_IMAGE_SAMPLERS
uniform sampler2D uSrc;
#else
layout(set=0, binding=0) uniform sampler uSampler;
layout(set=0, binding=1) uniform texture2D uSrc_;
#define uSrc sampler2D(uSrc_, uSampler)
#endif

uniform vec4 uColor;

The uColor uniform is valid GL330, but it isn't valid Vulkan, which needs to define set/binding and use "block style" uniforms.

Of course we could just do what we did above:

#ifdef COMBINED_IMAGE_SAMPLERS
uniform sampler2D uSrc;
uniform vec4 uColor;
#else
layout(set=0, binding=0) uniform sampler uSampler;
layout(set=0, binding=1) uniform texture2D uSrc_;
#define uSrc sampler2D(uSrc_, uSampler)
layout (set=0, binding=2) uniform uColor {
  vec4 color;
};
#endif

We should probably rename COMBINED_IMAGE_SAMPLERS to something like VULKAN_SEMANTICS or NON_VULKAN_SEMANTICS.

But now we're replicating every uniform. Maybe its still the best solution given the constraints discovered in this PR's code. But using a single "vulkan style" shader instead of doubling up on everything is starting to look more attractive.

Can you think of any other solutions to this problem?

@kvark
Copy link
Member

kvark commented May 10, 2020

I gave up too early the first time I tried and I just started diving into other solutions. I should have circled back with you when I couldn't get it to work.

Sorry, I just wrote this approximately from the way I understand it should work, not copied from actual code that works. Glad you got that working though!

The uColor uniform is valid GL330, but it isn't valid Vulkan, which needs to define set/binding and use "block style" uniforms.

WebGPU doesn't have free-standing uniforms (or push constants), so there needs to be a path where all the free uniforms are put into a uniform buffer. Whether this is the only path (which is compatible to both OpenGL and WebGPU), or an alternative path, is up to debate. Performance-wise, a free-standing uniform could theoretically be put into a scalar register by the driver (especially, AMD). I don't know for sure if they do this.

We should probably rename COMBINED_IMAGE_SAMPLERS to something like VULKAN_SEMANTICS or NON_VULKAN_SEMANTICS.

Vulkan supports both combined and non-combined image samplers, so the part about texture/sampler separation shouldn't be called "VULKAN_SEMANTICS".

But now we're replicating every uniform.

You shouldn't be replicating every uniform. As I mentioned, you'd just be putting all of them into a single uniform buffer. Note that in D3D11/12 there isn't even such a thing as a free-standing uniform, and all those would have to go into a "global" uniform buffer anyway.

@cart
Copy link
Author

cart commented May 10, 2020

Sorry, I just wrote this approximately from the way I understand it should work, not copied from actual code that works. Glad you got that working though!

You don' need to apologize. This was a @cart failure :)

Vulkan supports both combined and non-combined image samplers, so the part about texture/sampler separation shouldn't be called "VULKAN_SEMANTICS".

Fair point. So either we split them out into two separate defs or call it something like WEBGPU_SEMANTICS?

Everything else

I think I'm being dense. I understand that WebGPU needs all uniforms in uniform buffers and that it doesn't support push constants, but I don't see how this solves the "replication" problem in shaders.

uniform vec4 uColor; can't be compiled by glslangValidator in "vulkan semantics" mode. uniform uColor { vec4 color; }; can't be compiled in "GL330 mode". Therefore to support both don't we need to declare the uniform in the shader twice like I did in my second example above?

@cart
Copy link
Author

cart commented May 10, 2020

And we need to compile in "vulkan mode" for webgpu because "GL mode" doesn't support split samplers.

@kvark
Copy link
Member

kvark commented May 10, 2020

So either we split them out into two separate defs or call it something like WEBGPU_SEMANTICS?

Yep

uniform uColor { vec4 color; }; can't be compiled in "GL330 mode"

Why? OpenGL uniform buffer objects are supported in GL 3.1 as well as GLES 3.0. Therefore, it looks like using them can be the one and only code path supported everywhere.

@cart
Copy link
Author

cart commented May 11, 2020

Once again you're right. I was treating the output of this command as the source of truth:

glslangValidator -G330 shader.fs.glsl -DCOMBINED_IMAGE_SAMPLERS -S frag -E

Because thats what the master branch's shader makefile uses when it produces the gl3/gl4 glsl from glsl source files. I wasn't aware that this actually applies additional constraints (because the -G330 instructs it to produce a spirv binary?)

This command succeeds when I run it on a uniform buffer object with #version 330:

glslangValidator shader.fs.glsl -S frag -DCOMBINED_IMAGE_SAMPLERS -l

Is that the command you would use to validate? If so the makefile will need to be updated because it fails when a ubo is added with the current setup.

The webgpu spirv could then be generated by substituting a higher version number like 450 and passing in -V --auto-map-bindings

I think the only missing piece is metal support. Sadly I think the use of UBOs in this pr is what broke the "name based argument reflection" in MetalDevice. So that problem will likely still need solving.

@cart
Copy link
Author

cart commented May 11, 2020

* sigh *
I made one more mistake. The makefile only uses -G when generating the spirv used to produce MTL shaders. The makefile is fine as-is for glsl validation. It was failing because of the metal spv compilation.

@kvark
Copy link
Member

kvark commented May 11, 2020

You can write GLSL in the way that exposes a uniform buffer via a single global variable, similar to how Metal sees that. I just tested it on shader playground. The source GLSL has:

layout(set = 0, binding = 0) uniform Globals {
    vec4 color;
} u_Globals;

The output Metal shader has:

fragment main0_out main0(constant Globals& u_Globals [[buffer(0)]])

So the name "u_Globals" would be preserved this way.

@cart cart mentioned this pull request May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants