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

[WGSL] Overload resolution shouldn't eagerly promote variables #13017

Conversation

tadeuzagallo
Copy link
Member

@tadeuzagallo tadeuzagallo commented Apr 21, 2023

5186c24

[WGSL] Overload resolution shouldn't eagerly promote variables
https://bugs.webkit.org/show_bug.cgi?id=255771
rdar://108361197

Reviewed by Mike Wyrzykowski.

During overload resolution, when trying to assign a type to a variable we check
whether it satisifies the variable's constraints. In order to satisfy a constraint
we might need to promote the type being assigned to the variable. The example
from the tests is when the variable requires a concrete type but we are assigning
an abstract type to it. Originally, we would promote the type as soon as we assigned
it to the variable, but that is not always correct. The promotion algorithm will
pick the cheapest conversion, in this case it would promote an AbstractInt to an
i32, which is a valid conversion. However, later we observe that the same variable
is also being unified with an u32, which was also a valid conversion for the original
type (AbstractInt), but since we already promoted the variable to i32, we can no
longer unify the variable with u32, since we can't unify i32 and u32. The solution
is faily straightforward though: we still reject types that don't satisfy the constraints,
but hold off on promoting the type until we materialize the variable after determining
that a given overload is a viable candidate. This guarantees that there will be a
suitable promotion that will satisfy the constraints, but we delay choosing which
type we'll promote to until we have looked at all arguments.

* Source/WebGPU/WGSL/Overload.cpp:
(WGSL::OverloadResolver::materialize const):
(WGSL::OverloadResolver::assign):
* Source/WebGPU/WGSL/tests/valid/overload.wgsl:

Canonical link: https://commits.webkit.org/263355@main

37d466f

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@tadeuzagallo tadeuzagallo self-assigned this Apr 21, 2023
@tadeuzagallo tadeuzagallo added the WebGPU For bugs in WebGPU label Apr 21, 2023
@tadeuzagallo tadeuzagallo changed the title [WGSL] [WGSL] Overload resolution shouldn't eagerly promote variables Apr 21, 2023
@tadeuzagallo tadeuzagallo force-pushed the eng/WGSL-Overload-resolution-shouldnt-eagerly-promote-variables branch from db176cb to 561afa9 Compare April 21, 2023 13:44
Copy link
Contributor

@djg djg left a comment

Choose a reason for hiding this comment

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

:shipit:

@tadeuzagallo tadeuzagallo added merge-queue Applied to send a pull request to merge-queue and removed merge-queue Applied to send a pull request to merge-queue labels Apr 25, 2023
@tadeuzagallo tadeuzagallo force-pushed the eng/WGSL-Overload-resolution-shouldnt-eagerly-promote-variables branch from 561afa9 to 37d466f Compare April 25, 2023 07:57
@tadeuzagallo tadeuzagallo added the merge-queue Applied to send a pull request to merge-queue label Apr 25, 2023
https://bugs.webkit.org/show_bug.cgi?id=255771
rdar://108361197

Reviewed by Mike Wyrzykowski.

During overload resolution, when trying to assign a type to a variable we check
whether it satisifies the variable's constraints. In order to satisfy a constraint
we might need to promote the type being assigned to the variable. The example
from the tests is when the variable requires a concrete type but we are assigning
an abstract type to it. Originally, we would promote the type as soon as we assigned
it to the variable, but that is not always correct. The promotion algorithm will
pick the cheapest conversion, in this case it would promote an AbstractInt to an
i32, which is a valid conversion. However, later we observe that the same variable
is also being unified with an u32, which was also a valid conversion for the original
type (AbstractInt), but since we already promoted the variable to i32, we can no
longer unify the variable with u32, since we can't unify i32 and u32. The solution
is faily straightforward though: we still reject types that don't satisfy the constraints,
but hold off on promoting the type until we materialize the variable after determining
that a given overload is a viable candidate. This guarantees that there will be a
suitable promotion that will satisfy the constraints, but we delay choosing which
type we'll promote to until we have looked at all arguments.

* Source/WebGPU/WGSL/Overload.cpp:
(WGSL::OverloadResolver::materialize const):
(WGSL::OverloadResolver::assign):
* Source/WebGPU/WGSL/tests/valid/overload.wgsl:

Canonical link: https://commits.webkit.org/263355@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/WGSL-Overload-resolution-shouldnt-eagerly-promote-variables branch from 37d466f to 5186c24 Compare April 25, 2023 08:35
@webkit-commit-queue
Copy link
Collaborator

Committed 263355@main (5186c24): https://commits.webkit.org/263355@main

Reviewed commits have been landed. Closing PR #13017 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 5186c24 into WebKit:main Apr 25, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 25, 2023
@tadeuzagallo tadeuzagallo deleted the eng/WGSL-Overload-resolution-shouldnt-eagerly-promote-variables branch May 17, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebGPU For bugs in WebGPU
Projects
None yet
5 participants