[WGSL] Initial implementation of operator overload resolution #9763

tadeuzagallo · 2023-02-07T18:00:13Z

`387c1b1`

[WGSL] Initial implementation of operator overload resolution
https://bugs.webkit.org/show_bug.cgi?id=251863
rdar://105133289

Reviewed by Myles C. Maxfield.

First pass at adding support for overloaded operators and functions. For now,
only `+` is implemented with 2 operators, and declaring the operations is very
cumbersome, but the patch is fairly large as it is, so more convenient declarations
will follow in a separate patch.

* Source/WebGPU/WGSL/Overload.cpp: Added.
(WGSL::log):
(WGSL::logLn):
(WGSL::OverloadResolver::OverloadResolver):
(WGSL::OverloadResolver::resolve):
(WGSL::OverloadResolver::materialize const):
(WGSL::OverloadResolver::considerCandidates):
(WGSL::OverloadResolver::considerCandidate):
(WGSL::OverloadResolver::calculateCost):
(WGSL::OverloadResolver::unify):
(WGSL::OverloadResolver::assign):
(WGSL::OverloadResolver::resolve const):
(WGSL::OverloadResolver::conversionCost const):
(WGSL::primitive_pair):
(WGSL::OverloadResolver::conversionCostImpl const):
(WGSL::resolveOverloads):
(WTF::printInternal):
* Source/WebGPU/WGSL/Overload.h: Copied from Source/WebGPU/WGSL/Types.h.
* Source/WebGPU/WGSL/TypeCheck.cpp:
(WGSL::TypeChecker::TypeChecker):
(WGSL::TypeChecker::visit):
(WGSL::TypeChecker::chooseOverload):
* Source/WebGPU/WGSL/TypeStore.cpp:
(WGSL::TypeStore::TypeStore):
(WGSL::TypeStore::vectorType):
(WGSL::TypeStore::matrixType):
(WGSL::TypeStore::allocateConstructor):
* Source/WebGPU/WGSL/TypeStore.h:
* Source/WebGPU/WGSL/Types.h:
* Source/WebGPU/WGSL/WGSL.h:
* Source/WebGPU/WebGPU.xcodeproj/project.pbxproj:

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

06571ff

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-02-07T18:00:28Z

EWS run on previous version of this PR (hash 12103b6)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

litherum · 2023-02-10T06:48:13Z

Source/WebGPU/WGSL/Overload.cpp

+inline void log(Arguments&&... arguments)
+{
+    if (shouldDumpOverloadDebugInformation)
+        dataLog(logPrefix, std::forward<Arguments>(arguments)...);


litherum · 2023-02-10T06:51:12Z

Source/WebGPU/WGSL/Overload.cpp

+
+    FixedVector<std::optional<ViableOverload>> considerCandidates();
+    std::optional<ViableOverload> considerCandidate(const OverloadCandidate&);
+    unsigned calculateCost(const AbstractType&, Type*);


The way overload resolution works in WGSL doesn't have a cost function.

Firstly, only built-in functions are allowed to overload. User-written functions can't overload.

Secondly, all the built-in functions which are overloaded have mutually-exclusive signatures. There's never any situation where 2 function declarations are valid for a single call.

If you have something like sin(3.0), the literal 3.0 gets concretized according to concretization rules, which are not sensitive to the context that it's being passed to a function. So there's only a single correct overload after the concretization.

That's interesting. I tried to match the spec as closely as possible as described here: https://www.w3.org/TR/WGSL/#overload-resolution-section

Firstly, only built-in functions are allowed to overload.

This function doesn't assume anything about the overloads being user defined or builtins, but I don't see how assuming they are builtins wouldn't make the resolution any simpler.

Secondly, all the built-in functions which are overloaded have mutually-exclusive signatures. There's never any situation where 2 function declarations are valid for a single call.

That's true, but that doesn't mean that the same declaration can't yield more than one resolution. i.e. + (T, T) -> T apply to 1 + 1 can resolve T to AbstractInt, i32, u32, AbstractFloat, f32 or f16, in that order of preference according to the conversion rank table.

If you have something like sin(3.0), the literal 3.0 gets concretized according to concretization rules, which are not sensitive to the context that it's being passed to a function. So there's only a single correct overload after the concretization.

You know the spec better than me, but that's not how I interpreted it. The way I understood it, overload resolution doesn't concretize types. e.g. const x = sin(3.0), x should have type AbstractFloat, but if we use var y = sin(3.0), now y should have type f32, according to the conversion rank function.

Maybe that's some nuance that I'm missing, and it can be implemented in a simpler way than described in the spec.

This function doesn't assume anything about the overloads being user defined or builtins, but I don't see how assuming they are builtins wouldn't make the resolution any simpler.

It's a finite set - every member of every set of overload resolutions is known before we look at the first line of the user program.

That's interesting. I tried to match the spec as closely as possible as described here: https://www.w3.org/TR/WGSL/#overload-resolution-section

https://gpuweb.github.io/gpuweb/wgsl/#conversion-rank is totally new to me, I had no idea that there was a ranking algorithm. My background knowledge here comes from the fact that the CG wanted to forbid user functions from overriding each other, precisely because (IIRC) they didn't want to write a general ranking algorithm - they wanted to punt on that algorithm until after the initial version of the language. I guess my memory is out-of-date, since it seems pretty clear that a general ranking algorithm exists.

Sorry for the noise, I think your following the spec was the right thing to do.

litherum · 2023-02-10T06:51:34Z

Source/WebGPU/WGSL/Overload.cpp

+    FixedVector<std::optional<ViableOverload>> considerCandidates();
+    std::optional<ViableOverload> considerCandidate(const OverloadCandidate&);
+    unsigned calculateCost(const AbstractType&, Type*);
+    unsigned conversionCost(Type*, Type*) const;


WGSL types don't have implicit conversions, except for literals, so I'm not sure this is the right approach.

This is supposed to match 1:1 the "Conversion Rank" table in the spec: https://www.w3.org/TR/WGSL/#conversion-rank

I should rename this to conversionRank if we end up landing this.

litherum

Looks pretty straightforward.

litherum · 2023-02-10T08:48:32Z

Source/WebGPU/WGSL/TypeCheck.cpp

+        TypeVariable T { 0, TypeVariable::Number };
+        plus1.typeVariables.append(T);
+        plus1.parameters.append(T);
+        plus1.parameters.append(T);
+        plus1.result = T;


Is there a way to represent this as data rather than C++? I assume there are going to be dozens of more stanzas like this. Can we generate them at build time? (or run time?)

Yeah, that's the plan. I'm hoping to do something similar to JSC's BytecodeList.rb, where we declare all these in a file and output these C++ definitions at compile time.

litherum · 2023-02-10T08:50:07Z

Source/WebGPU/WGSL/WGSL.h

@@ -47,15 +47,15 @@ class ShaderModule;
 struct SuccessfulCheck {
    SuccessfulCheck() = delete;
    SuccessfulCheck(SuccessfulCheck&&);
-    SuccessfulCheck(Vector<Warning>&&, UniqueRef<ShaderModule>&&);
+    SuccessfulCheck(WTF::Vector<Warning>&&, UniqueRef<ShaderModule>&&);


Can we rejigger our includes to make this not have to be changed?

Yeah, that was 100% a mistake, I'll upload a patch to move the types into their own namespace.

Preferably WGSL::Type::Vector

Yes, I moved it in #9976. Only I made it Types, because I wanted Type to remain the base class that lives in the top-level WGSL namespace.

webkit-early-warning-system · 2023-02-14T10:49:19Z

EWS run on previous version of this PR (hash 6bbec1a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2	⏳ 🛠 jsc-armv7
	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	⏳ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-02-14T11:05:27Z

EWS run on previous version of this PR (hash cef64cd)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-02-14T14:05:50Z

EWS run on current version of this PR (hash 06571ff)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=251863 rdar://105133289 Reviewed by Myles C. Maxfield. First pass at adding support for overloaded operators and functions. For now, only `+` is implemented with 2 operators, and declaring the operations is very cumbersome, but the patch is fairly large as it is, so more convenient declarations will follow in a separate patch. * Source/WebGPU/WGSL/Overload.cpp: Added. (WGSL::log): (WGSL::logLn): (WGSL::OverloadResolver::OverloadResolver): (WGSL::OverloadResolver::resolve): (WGSL::OverloadResolver::materialize const): (WGSL::OverloadResolver::considerCandidates): (WGSL::OverloadResolver::considerCandidate): (WGSL::OverloadResolver::calculateCost): (WGSL::OverloadResolver::unify): (WGSL::OverloadResolver::assign): (WGSL::OverloadResolver::resolve const): (WGSL::OverloadResolver::conversionCost const): (WGSL::primitive_pair): (WGSL::OverloadResolver::conversionCostImpl const): (WGSL::resolveOverloads): (WTF::printInternal): * Source/WebGPU/WGSL/Overload.h: Copied from Source/WebGPU/WGSL/Types.h. * Source/WebGPU/WGSL/TypeCheck.cpp: (WGSL::TypeChecker::TypeChecker): (WGSL::TypeChecker::visit): (WGSL::TypeChecker::chooseOverload): * Source/WebGPU/WGSL/TypeStore.cpp: (WGSL::TypeStore::TypeStore): (WGSL::TypeStore::vectorType): (WGSL::TypeStore::matrixType): (WGSL::TypeStore::allocateConstructor): * Source/WebGPU/WGSL/TypeStore.h: * Source/WebGPU/WGSL/Types.h: * Source/WebGPU/WGSL/WGSL.h: * Source/WebGPU/WebGPU.xcodeproj/project.pbxproj: Canonical link: https://commits.webkit.org/260254@main

webkit-commit-queue · 2023-02-14T15:23:18Z

Committed 260254@main (387c1b1): https://commits.webkit.org/260254@main

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

tadeuzagallo self-assigned this Feb 7, 2023

tadeuzagallo added the WebGPU For bugs in WebGPU label Feb 7, 2023

tadeuzagallo requested review from litherum, mwyrzykowski, djg and grorg February 7, 2023 18:35

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 8, 2023

litherum reviewed Feb 10, 2023

View reviewed changes

litherum approved these changes Feb 10, 2023

View reviewed changes

tadeuzagallo force-pushed the eng/WGSL-Initial-implementation-of-operator-overload-resolution branch from 12103b6 to 6bbec1a Compare February 14, 2023 10:49

tadeuzagallo force-pushed the eng/WGSL-Initial-implementation-of-operator-overload-resolution branch from 6bbec1a to cef64cd Compare February 14, 2023 11:03

tadeuzagallo force-pushed the eng/WGSL-Initial-implementation-of-operator-overload-resolution branch from cef64cd to 06571ff Compare February 14, 2023 14:05

tadeuzagallo added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Feb 14, 2023

webkit-early-warning-system force-pushed the eng/WGSL-Initial-implementation-of-operator-overload-resolution branch from 06571ff to 387c1b1 Compare February 14, 2023 15:23

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 14, 2023

webkit-early-warning-system merged commit 387c1b1 into WebKit:main Feb 14, 2023

tadeuzagallo deleted the eng/WGSL-Initial-implementation-of-operator-overload-resolution branch May 17, 2023 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WGSL] Initial implementation of operator overload resolution #9763

[WGSL] Initial implementation of operator overload resolution #9763

tadeuzagallo commented Feb 7, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 7, 2023 •

edited

litherum Feb 10, 2023

litherum Feb 10, 2023

tadeuzagallo Feb 10, 2023

litherum Feb 10, 2023

litherum Feb 10, 2023

tadeuzagallo Feb 10, 2023

tadeuzagallo Feb 10, 2023

litherum left a comment

litherum Feb 10, 2023

tadeuzagallo Feb 10, 2023

litherum Feb 10, 2023

tadeuzagallo Feb 10, 2023

djg Feb 14, 2023

tadeuzagallo Feb 14, 2023

webkit-early-warning-system commented Feb 14, 2023 •

edited

webkit-early-warning-system commented Feb 14, 2023 •

edited

webkit-early-warning-system commented Feb 14, 2023 •

edited

webkit-commit-queue commented Feb 14, 2023

[WGSL] Initial implementation of operator overload resolution #9763

[WGSL] Initial implementation of operator overload resolution #9763

Conversation

tadeuzagallo commented Feb 7, 2023 • edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litherum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Feb 14, 2023 • edited

webkit-early-warning-system commented Feb 14, 2023 • edited

webkit-early-warning-system commented Feb 14, 2023 • edited

webkit-commit-queue commented Feb 14, 2023

tadeuzagallo commented Feb 7, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 7, 2023 •

edited

webkit-early-warning-system commented Feb 14, 2023 •

edited

webkit-early-warning-system commented Feb 14, 2023 •

edited

webkit-early-warning-system commented Feb 14, 2023 •

edited