Add optional feature dual_source_blending

[Jiawei-Shao]

This patch adds dual_source_blending to WebGPU and WGSL SPEC.

• Add dual_source_blending as an optional feature in both WebGPU and WGSL.
• Support @blend_src in WGSL when dual_source_blending is enabled.
• Add all the blend factors that use src1 to WebGPU SPEC.

Fixed: #4283

[github-actions]

Previews, as seen when this build job started (0f7d944):
WebGPU _{webgpu.idl | Explainer | Correspondence Reference}
WGSL _{grammar.js | wgsl.lalr.txt}

[alan-baker]

Is there value to having this default? Is there some code generation concern that makes this desirable? It seems like it would be clearer to not have it

[Jiawei-Shao]

Here I just refer the statement in

• Metal (Page 109): If index(i) is not specified in the attribute, the default is an index of 0
• Vulkan: variables declared without an Index decoration are considered to have an Index decoration of zero

Setting a default value can also make us be able to avoid saying something like The [=blend_src=] attribute of |output| is omitted or 0 (we can directly say The [=blend_src=] attribute of |output| is 0).

[alan-baker]

Maybe I'm misunderstanding how this is used. Here is my understanding, please correct me if it's wrong.

1. Non-dual source blending shader

• Same as today. No new attributes, just location on outputs.
• Don't think about blend_src at all

2. Dual source blending shader

• blend_src(1) must be specified on output location 0
• The must only be two outputs at location 0
• Therefore, omitting blend_src(0) is saving a bit of typing, but resulting is less clarity

Is there a case where you could specify blend_src(0), but not blend_src(1)? If not then the only other reason I could see for the default would be to aid code generation, but I'm having trouble envisioning that scenario.

[Jiawei-Shao]

Thanks for your explanation and now I fully understand your meaning. I think you are right and I agree with you.

[Jiawei-Shao]

So if we decide to omit blend_src(0), then I think we can just use blend_src1 instead of the pattern blend_src1(num). The shader will look like:

struct FragOut {
    @location(0) color : vec4<f32>,
    @location(0) @blend_src1 blend : vec4<f32>,
}

What do you think?

[dj2]

@blend_src1 seems a lot less consistent with the rest of WGSL. @blend_src(1) matches up with the @location(0) much nicer. It also means we can have other blend numbers if the need ever arose in the future.

[Jiawei-Shao]

I've removed the default value of blend_src. PTAL, thanks!

[alan-baker]

I think this needs reworked. Above we stated that locations must not overlap in a structure. Maybe add to the structure bullet "unless the blend_source attribute is specified".

[Jiawei-Shao]

Now I try to fix this issue by specifying a location with either a [=attribute/location=] or both [=attribute/location=] and [=attribute/blend_src=]. What do you think?

[kainino0x]

Still some issues in the validation algorithm; it was too complicated to try to explain what should be done so I've pushed a commit. Please review my changes!

[Jiawei-Shao]

Thanks @kainino0x and @alan-baker !

I've merged the main branch into the PR and fixed several nits in Kai's comment. PTAL ,thanks!

[kainino0x]

fixed several nits in Kai's comment

Thanks for the fixes!

API text LGTM (except that I wrote part of it) but I haven't checked if it matches the validation in the backends. Will need tests.

[toji]

One request for the WebGPU spec, but overall it's looking good! I'm not in a good position to evaluate the WGSL spec changes.

[toji]

Thanks for adding the feature column to the blend factor table! LGTM, though I'll wait for someone on the WGSL spec side to give final approval.

[Kangz]

GPU Web WG 2024-05-08 Pacific-time

• JS: PR still needs work.
• KN: Make sure we have a milestone on it? Oh, the associated issue is already Milestone 1. Added that milestone to the PR.

[kdashg]

WGSL 2024-05-14 Minutes

• (PR is still in-progress)
• AB: Regarding naming, naming in WGSL doesn’t use short forms like ‘src’. Was that taken into consideration when designed?
• JB: don’t think we took that into consideration, but we did bikeshed it somewhat. Personally I like full words like ‘source’, but i don’t want to reopen if others don’t.
• KG: This has long history attached to this, prefer to not reopen. If you want to relitigate it, need a volunteer to champion that.
• AB: That’s fine. Let’s leave this, remember the WGSL style in the future.

[alan-baker]

The attribute just needs reordered in the overall list, but otherwise LGTM.

[Jiawei-Shao]

It still needs to be sorted alphabetically.
@alan-baker Done

[alan-baker]

WGSL changes LGTM

[kainino0x]

API spec LGTM. Not sure about WGSL spec though

[kainino0x]

algorithm is required to make variables work nicely. Also it's clearer to associate these two paragraphs together.

However, apologies, I'm rereading this (and the text I suggested) and realizing that I totally misunderstood what the second bullet is about - it's about inputs, not outputs. I guess just looking at structs is good enough for outputs (since a struct is the only way to have more than one output of an entry point, currently, I think).

Suggested change

	For each set of outputs |outputs| in the following sets:
	* Members within a structure type.
	This applies to any structure, not just those used in shader stage inputs or outputs.
	* An entry point's shader stage inputs,
	i.e. locations for its formal parameters, or for the members of its formal parameters of structure type.
	The following [=shader-creation error|must=] be `true`:
	- |outputs| [=shader-creation error|must not=] contain two outputs with the same location and without a given [=attribute/blend_src=] attribute.
	- If any output in |outputs| specifies a [=attribute/blend_src=] attribute:
	|outputs| must contain exactly 2 outputs, one with `@location(0) @blend_src(0)` and one with `@location(0) @blend_src(1)`.
	<div algorithm="locations in entry point inputs">
	For each entry point defined in a WGSL module, let |inputs| be its set of shader stage inputs
	(i.e. locations for its formal parameters, or for the members of its formal parameters of structure type).
	- |inputs| [=shader-creation error|must not=] contain two entries with the same [=attribute/location=] value.
	</div>
	<div algorithm="locations in structs">
	For each structure type defined in a WGSL module (not just those used in shader stage inputs or outputs),
	let |members| be the set of struct members with [=attribute/location=] attributes.
	- |members| [=shader-creation error|must not=] contain two entries without [=attribute/blend_src=]
	attributes that have the same with the same [=attribute/location=] value.
	- If any entry in |members| specifies a [=attribute/blend_src=] attribute:
	- |members| [=shader-creation error|must=] contain exactly 2 entries,
	one with `@location(0) @blend_src(0)` and one with `@location(0) @blend_src(1)`.
	</div>

@alan-baker WDYT, does this seem correct?

[alan-baker]

Maybe it would be better flipping the structs algorithm bullets ordering:

• If any entry in |members| specifies a blend src attribute:
  • members must contain exactly two entries ...
• Otherwise, members not contain two entries with the same location value.

[Jiawei-Shao]

Done

[teoxoy]

The current validation code will check that if the pipeline is expecting dual-source blending, the shader should as well but we should also check that if the shader expects dual-source blending, the pipeline does as well.

[Jiawei-Shao]

But we should also check that if the shader expects dual-source blending, the pipeline does as well.

Actually according to our tests we can write outputs to @blend_src(1) in shader while on the API side the blend factor doesn't use Src1.

[teoxoy]

I was concerned about it because HLSL just reuses SV_Target1 for the 2nd blend src of target 0 (there is no distinction between the blend src and render target index).

But it seems it doesn't matter, it will get ignored either way.