I believe that the SPIR-V backend to dxc has had to solve a similar problem:

https://github.com/Microsoft/DirectXShaderCompiler/blob/master/docs/SPIR-V.rst#hlsl-register-and-vulkan-binding

Was the approach here considered?

@damyanp the approach you linked to requires changing the source (by adding the "[[vk::binding(X[, Y])]]" attributes), which is something we are trying to avoid in order to preserve compatibility with existing code bases.

@litherum thank you for writing this up!

This mapping is provided in the GPUShaderModule so compilation can be done as early as possible and doesn't have to be deferred until pipeline creation time.

I think some of the backends (Dx12, Metal-without-argument-buffers) would still require us to defer the shader creation to the pipeline creation time, since this is where the layout is known. But having the API shaped in a way that at least allows an implementation to do this work ahead of time is good.

const overriddenBindPoints = {"register(u2, space1)": {binding: 3, bindGroup: 4}};

If the "space" qualifier is already provided, wouldn't we want to just use it directly? In what case would an extra mapping be needed? My understanding was that we only want to do this to support shaders that were written for DX11 resource model to begin with. Did you consider just mapping "u2", "t0", etc as a key instead of the whole "register(x, y)"?

@kvark - dxc has several strategies, one of which involves changing the source code. There's also a bunch of command line parameters that can be passed to dxc to control the policy of how the registers are remapped. My thinking is that anyone who already has shaders that they're compiling with dxc but using with vulkan will have figured out what parameters they need to pass to dxc in order to configure a set of registers that work with that style of binding model.

Can you help me understand why DX12 would need to understand the layout in order to compile the shader? In DX12 the shader can be compiled to bytecode without knowing the layout. When the bytecode is passed to the driver while creating the pipeline state object the layout is required. Or am I missing something?

BTW, we've already talked about what DXC does for both D3D12 and for Vulkan in #188 and #192.

Offhand, I think you (@damyanp) are right that this mapping could be at pipeline creation instead of shader module creation. (In the world, of course, where WHLSL is what's used to create a shader module.)

@damyanp command line options is not a choice we could consider here, since no command line is involved. And the fact that the users already know the mapping they need is one of the reasons we went with Vulkan-like binding model to begin with. This PR is making a concrete proposal on how the users would communicate this information (that they already know) to us.

Can you help me understand why DX12 would need to understand the layout in order to compile the shader?

Problem originates from the limitation of DX12 that samplers and non-samplers can live in the same descriptor heap. Before calling SPIRV-Cross we (in gfx-rs) are setting up the resource overrides accordingly, and this is where the pipeline layout is needed. That is to say, it's been a while since I looked in depth at that logic piece, so we might find a way to improve that.

They could be arguments in a dictionary instead of actual command line args. But the actual semantics of those command line arguments (assuming you're talking about -fvk-s-shift & co) is very compat-focused, and doesn't seem like a reasonable first-class API.

I would argue that requiring developers to manually list out a mapping for all shader registers is also very compat focused. The whole point of this overrides API is to provide a way to adapt a shader written for one binding model to work with a different binding model. We would also expect that any shader specifically written for WebGPU would not need this modification applied.

My expectation here is that the implementation for this would effectively modify the source code that's passed to the WHLSL compiler, and could easily be implemented outside of WebGPU.

Given this, I'm unsure that this should be a first-class API.

If duplicate entries appear in the sequence, the first one wins.

nit: Would make more sense to me to just make it invalid. Also, that semantic or requirement should be in the spec (as comment or prose)

We would also expect that any shader specifically written for WebGPU would not need this modification applied.

Correct; this entire API is optional.

could easily be implemented outside of WebGPU

Not without a parser written in JS/WASM, which places an undue burden on web developers.

Not without a parser written in JS/WASM, which places an undue burden on web developers.

Well, that only has to be written once :) This translation could happen even earlier on in the process - before the shader is even loaded into the browser, for example.

I would like to see WebGPU and WHLSL agree on the binding model. If we want to provide paths to help port non-WHLSL shaders to WHLSL then that's great, but I don't think that this conversion belongs in WebGPU.

This is basically the crux of what I wanted to say in the meeting (but didn't have time). While HLSL compatibility is important to some users of the API, having incompatibilities only in bindings seems acceptable to me - it's a tiny volume of changes for an existing engine, especially given they have to also rewrite all of the non-shader code.

For those that really want to run totally unmodified, they're going to have to use a translation layer anyway, and that translation layer would be able to do this extra work.

I would like to see WebGPU and WHLSL agree on the binding model.

I, too, would like to see WebGPU and WHLSL agree on the binding model. However, I believe that ship has sailed in #188.

I'd like to continue seeing if we can find a way to have WHLSL feel like a more natural shading language for WebGPU.

On another note though, I'm wondering how we'd expect someone who has a load of HLSL shaders that they compile with dxc to spir-v using the various -fvt-*-shift compiler options generate the appropriate mapping tables to work with this API? It looks like they'd have to build up a mapping table listing every possible remapping. Can we improve on this?

Discussed at 8 July 2019 teleconference

Rebased over #365 (and squashed, sorry)

This was discussed at the 15 July 2019 teleconference.

I think we all agreed to close this and make the changes in WHLSL. Closing!