[RFC] Input / output proposal

[Jasper-Bekkers]

No description provided.

[Jasper-Bekkers]

Rendered

[khyperia]

Thanks for doing this, this looks really awesome!

[khyperia]

Should (all) these attributes be #[spirv(render_target = 0)] to be consistent with the rest of the attributes?

[Jasper-Bekkers]

I thought we had consensus that we shouldn't do that #128 and I was under the impression that most of those #[spirv] attributes were either temporary or to get at specific implementation specific things from spirv. Not for higher level features.

[khyperia]

The conclusion of 128 is that we're keeping spirv for now, and perhaps we rename it to #[shader(render_target = 0)] in the future. Besides, there's significant technical difficulties in supporting #[render_target(0)], and is the reason #[spirv(...)] was invented in the first place.

[XAMPPRocky]

Yeah, I agree even if it's not called spirv there should be at least one wrapper attribute around the inner attribute. It would get confusing as a user as to who is defined the attribute.

[Jasper-Bekkers]

Ah right, I was thinking of these along the same lines as the regular rustc built-in attributes. However, if we feel putting these in their own namespace is the best way forward then I think we should do that. I would suggest putting them in another namespace then the spriv namespace though, mostly to keep them separate and avoid potential name clashes, but also to keep the new attribute clean and only add things there after some deliberation (instead of straight up forwarding spirv_headers symbols) and so we can avoid naming things after spirv-isms such as gl_compute or the confusion between workgroup and subgroup (two separate but very similar concepts in spirv).

[khyperia]

I would suggest putting them in another namespace then the spriv namespace though, mostly to keep them separate and avoid potential name clashes

I strongly disagree, the conclusion of #128 is that we're keeping things in #[spirv(..)] for now. If you believe the current system is unclean and has issues like potential name clashes, then let's address those in another MCP, instead of patching on half-fixes like new namespaces.

so we can avoid naming things after spirv-isms such as gl_compute

I am fully aware of the issues with this and have asked feedback on names, and multiple times the conclusion has been that the current system is fine. If we want to change those decisions, let's open another thread.

[khyperia]

Should these be by-value instead of by-ref? Having them be by-ref significantly complicates the implementation and generates suboptimal SPIR-V.

[Jasper-Bekkers]

I think by-ref here is an omission on my part, I think by-value doesn't make this code any more or less usable from a user's point of view, so by all means if it makes life easier on our end by-value it is.

[khyperia]

What behavior do you imagine when attributes are missing? Error, implicit default value, something else?

[Jasper-Bekkers]

I think it's most likely that all of the members of a #[stage_in] struct will have to have some kind of decoration - either #[position], #[vertex_id], etc or #[attribute(..)]. However, I don't think it's required for #[stage_in] to have to have an #[attribute(..)] (see below).

[charles-r-earp]

Potentially #[spirv(attribute="position")] or #[spirv(attribute=0)] instead?

[Jasper-Bekkers]

The reason suggested much sorter syntax for most of these things is because they are quite common in shader cod and pop up all the time.

@charles-r-earp the word attribute here refers to a specific kind of attribute which is modeled in Vulkan by something known as the vertex input attributes, adding position in there would complicate things I feel because that's one of the outputs of a vertex shader.

[Jasper-Bekkers]

This may be an indicator that the word attribute is badly chosen for this case since it's already such an overloaded term, maybe vertex_attribute would be better, what do you think?

[khyperia]

The reason suggested much sorter syntax for most of these things is because they are quite common in shader cod and pop up all the time.

We've already decided to go with #[spirv(...)] in previous discussions, bikeshedding over this isn't useful. If we want to change that decision, let's open a separate MCP with drawbacks/advantages/alternatives and discuss it there.

However, I don't think it's required for #[stage_in] to have to have an #[attribute(..)] (see below).

I don't quite understand this comment, could you explain this more? What do you mean by #[stage_in] having an #[attribute(..)]?

[khyperia]

Just checking, does this replace the semantics for builtins, and all of those builtins will be supported, in both input+outputs?

[Jasper-Bekkers]

Only a relatively small amount of those builtins actually make sense in this context - for example one can't output a VertexId and some of those are shader-stage specific like TessLevelOuter or LocalInvocationId so I would suggest we curate a list of attributes that we allow as inputs and outputs per shader stage.

[khyperia]

Does SPIR-V support doing this packing like this built-in, or is this something we need to implement ourselves? If ourselves, what's the justification? Seems like a really obscure feature.

[Jasper-Bekkers]

Vec4 -> RGBA8 already happens automatically of course. And it's relatively common to be (bit) packing data into render-targets (example: gbuffer / vbuffer laydown). However, judging by the sketchy support shader-playground we probably should just only support f32 (and vectors thereof) types in fragment shader outputs.

[khyperia]

I'm confused, the shader-playground link doesn't have the same packing thing here, and so I'm not sure if you're saying that SPIR-V supports doing this packing built-in, or if we have to implement it ourselves. To be clear, I'm talking about having >1 field mapping to the same render target, and concatenating those fields into a single output.

[Jasper-Bekkers]

To be clear, I'm talking about having >1 field mapping to the same render target, and concatenating those fields into a single output.

Ah - I thought you meant the part about packing stuff together with u8 types and such. From the looks of it, it looks like the Component decoration isn't valid for render-target outputs and so all packing should be done manually.

In an earlier draft of this I actually had the pixel shader return a fixed-length array of (float|vec) from the pixel shader, something along these lines, which I feel is quite a bit simpler, but might run into trouble when outputting to render-targets with different components, or when outputting depth for example.

fn ps_main(#[stage_in] input: &VertexOutput, shading: &ShadingInputs, indirect_lighting: &IndirectLighting) -> [Vec4; 2] {
    let mut T = brdf(&shading);
    T += input.color;
    T += gi(&indirect_lighting);

    [
        pack_gbuffer0(T, material_idx),
        pack_gbuffer1(normal, specular_exponent),
    ]
}

I think maybe the best option could be to have the ps output a struct where each member represents one render target. What do you think?

[khyperia]

Is it valid to not have a #[spirv(stage_in)] attribute? No return value?

[Jasper-Bekkers]

We should probably require both for now, I think there might be some obscure use-cases for vertex shaders so we can try to list them here and see.

• Procedural full-screen quad will require VertexId, and so will need to declare that in #[stage_in] through something like a #[vertex_id] attribute. This would imply that at least #[attribute(N)] would be optional.
• It might be wanted occasionally would want to store out vertex data to a buffer (think debugging tools doing mesh visualization) - in this case you probably wouldn't want to run a pixel shader directly after (or maybe you would). However, this would probably require stream-out support (which we don't have, and probably won't have for a long time since it's quite an obscure feature).
• Another (much more likely) case would be shadow-map rendering. In this case you don't bind a pixel shader, but still need to output position from the vertex shader.

I think it's safe to require them both, unless somebody has another valid use-case that I didn't think of?

[XAMPPRocky]

Suggested change

In HLSL shader stages are linked by semanics, and can be incomplete (eg. one stage can export more then the next shader stage would import). While the semantics are documented and can be named; they don't actually imply anything other then which elements need to match up. Lots of semantics are available (`COLOR`, `BLENDWEIGHTS` etc) however, typically only `TEXCOORD[n]` is used to keep things positional and simple.

In HLSL shader stages are linked by semanics, and can be incomplete (eg. one stage can export more than the next shader stage would import). While the semantics are documented and can be named; they don't actually imply anything other then which elements need to match up. Lots of semantics are available (`COLOR`, `BLENDWEIGHTS` etc) however, typically only `TEXCOORD[n]` is used to keep things positional and simple.

[khyperia]

Considering this has been stuck at "yeah I'll do this in a couple months" for... half a year, I don't think this is ever going to get worked on, and we're going to have to live with #416