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.

Sign up for GitHub

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

Jump to bottom

Add optional feature dual_source_blending #4621

Merged
merged 43 commits into from
Jun 18, 2024
Merged

Add optional feature dual_source_blending #4621

merged 43 commits into from
Jun 18, 2024

Conversation

Jiawei-Shao
Copy link
Contributor

@Jiawei-Shao Jiawei-Shao commented May 6, 2024 •
edited
Loading

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

alan-baker
alan-baker reviewed May 6, 2024
View reviewed changes
wgsl/index.bs Outdated Show resolved Hide resolved
@alan-baker alan-baker added wgsl WebGPU Shading Language Issues api WebGPU API labels May 6, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2024 •
edited
Loading

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

kainino0x
kainino0x reviewed May 7, 2024
View reviewed changes
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
alan-baker
alan-baker reviewed May 7, 2024
View reviewed changes
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
kainino0x
kainino0x reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@Jiawei-Shao
Copy link
Contributor Author

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 kainino0x requested a review from toji May 8, 2024 17:59
@kainino0x
Copy link
Contributor

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
toji reviewed May 8, 2024
View reviewed changes
Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

spec/index.bs Show resolved Hide resolved
@kainino0x kainino0x added this to the Milestone 1 milestone May 8, 2024
@toji
Copy link
Member

toji commented May 9, 2024

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.

alan-baker
alan-baker reviewed May 10, 2024
View reviewed changes
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
wgsl/index.bs Outdated Show resolved Hide resolved
@Jiawei-Shao
Require all the @location(0) @blend_src(N) outputs have same type
df42b25 
As is required by Metal Shading Language:
```
5.2.3.5 Fragment Function Output Attributes
Multiple elements in the fragment function return type that use the
same color attachment index for blending needs to be declared with
the same data type.
```
teoxoy
teoxoy reviewed May 29, 2024
View reviewed changes
spec/index.bs Show resolved Hide resolved
@Jiawei-Shao
Copy link
Contributor Author

Thanks @alan-baker!

@kainino0x could you take a look?

dneto0
dneto0 requested changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good.

I have one suggestion: forbid use of blend_src without also having a location attribute.

wgsl/index.bs Outdated Show resolved Hide resolved
teoxoy
teoxoy approved these changes Jun 4, 2024
View reviewed changes
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks consistent with what we previously agreed on. 👍

@kdashg
Copy link
Contributor

kdashg commented Jun 4, 2024

WGSL 2024-06-04 Minutes
  • (KG: Are we happy with the WGSL half of the PR?)
  • DN: Was okay with this. Requested to restrict blend-source so that’s only usable when you have a location to use with it. Let’s make a rule to make sure people don’t think they’re getting blend-source when they can’t: forbid it when it’s not meaningful.
  • AB: That’s restating rules that appear later on in the entry points section, but I don’t mind the rules appearing in the attribute section too. Otherwise I signed off on this after some review
  • KG: Teo approved this too.
  • DS: So are we going to land?
  • JB: Ok to land from our perspective
  • DS: Need CTS?
  • KG: Nice to have CTS, but don’t block on it. (Expect it won’t change due to CTS).
  • MARK AS WGSL RESOLVED. LAND after API side happy with it.

@kdashg kdashg added the wgsl resolved Resolved - waiting for a change to the WGSL specification label Jun 4, 2024
kainino0x
kainino0x approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

wgsl/index.bs Outdated Show resolved Hide resolved
@kainino0x kainino0x requested a review from dneto0 June 7, 2024 22:27
dneto0
dneto0 approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Sorry for the delay.

@Jiawei-Shao
Copy link
Contributor Author

@kainino0x can we merge this PR right now?

@Kangz
Copy link
Contributor

Kangz commented Jun 18, 2024

Yes we can merge.

@kainino0x kainino0x merged commit 6eeebff into gpuweb:main Jun 18, 2024
4 checks passed
@kainino0x kainino0x mentioned this pull request Jun 20, 2024
Merged
@Kangz
Copy link
Contributor

Kangz commented Jun 25, 2024

GPU Web WG 2024-05-29 Atlantic-time
  • JS: discussed this feature last year. Some issues while creating the PR. Kai raised a blocking issue.
  • KN: trying to figure out validation rules in the WGSL spec. If you put blend_src on anything, you need exactly 2 locations? Blend sources 0 and 1, on the entry point? And different locations, can't use blend_src at all? Validate this eagerly in WGSL? Trying to figure out how to actually write this down.
  • JS: agree, no big issues, just spec wording.
  • KG: anything you wanted feedback from the group on?
  • JS: only updated the PR, found some new info on Mac/Metal - outputs have to have the same type when doing dual-source blending, or it will error. Simplifies this PR a lot, actually.
  • KG: need approvals on the PR.
  • KN: needs editorial work; we'll bring this back here, maybe don't need to wait for a Pacific time meeting.

juj added a commit to juj/wasm_webgpu that referenced this pull request Sep 6, 2024
@juj
IDL: Add support for dual source blending. gpuweb/gpuweb#4621. gpuweb…
0611911 
…/gpuweb#4710
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Devwanabe Devwanabe Devwanabe left review comments

@toji toji toji left review comments

@dj2 dj2 dj2 left review comments

@alan-baker alan-baker alan-baker approved these changes

@teoxoy teoxoy teoxoy approved these changes

@dneto0 dneto0 dneto0 approved these changes

@kainino0x kainino0x kainino0x approved these changes

Assignees
No one assigned
Labels
api WebGPU API wgsl resolved Resolved - waiting for a change to the WGSL specification wgsl WebGPU Shading Language Issues
Projects
None yet
Milestone
Milestone 1
Development

Successfully merging this pull request may close these issues.

Dual Source Blending Investigation + Feature Proposal
10 participants
@Jiawei-Shao @kainino0x @toji @Kangz @kdashg @dj2 @dneto0 @teoxoy @alan-baker @Devwanabe