[WGSL] Add support for multiplication #9043

tadeuzagallo · 2023-01-24T15:30:15Z

`f10f7a6`

[WGSL] Add support for multiplication
https://bugs.webkit.org/show_bug.cgi?id=251088
<rdar://problem/104600637>

Reviewed by Myles C. Maxfield and Dean Jackson.

As the title says: add parsing and a new BinaryOperation for multiplication.
The lexing will have to change as we add support for comments and other operators,
but this will do for now and enables testing more complex shaders.

* Source/WebGPU/WGSL/AST/ASTBinaryExpression.h:
* Source/WebGPU/WGSL/AST/ASTStringDumper.cpp:
(WGSL::AST::StringDumper::visit):
* Source/WebGPU/WGSL/Lexer.cpp:
(WGSL::Lexer<T>::lex):
* Source/WebGPU/WGSL/Metal/MetalFunctionWriter.cpp:
(WGSL::Metal::FunctionDefinitionWriter::visit):
* Source/WebGPU/WGSL/Parser.cpp:
(WGSL::Parser<Lexer>::parseMultiplicativeExpression):
* Source/WebGPU/WGSL/Token.cpp:
(WGSL::toString):
* Source/WebGPU/WGSL/Token.h:

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

879623b

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
✅ 🧪 webkitpy	✅ 🧪 api-ios	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
~~🛠 🧪 jsc~~	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
~~🛠 🧪 jsc-arm64~~	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
✅ 🧪 services	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
✅ 🛠 🧪 merge	✅ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

nt1m · 2023-01-24T16:12:26Z

Source/WebGPU/WGSL/AST/ASTBinaryExpression.h

@@ -31,6 +31,7 @@ namespace WGSL::AST {

 enum class BinaryOperation : uint8_t {
    Add,
+    Times,


Is it really named Times in the spec? otherwise I'd just use Multiply or smtg similar, though Myles might have a different opinion

The spec doesn't name it AFAICT, but since it's the token and not the operation I was between Times and Asterisk, but I saw times in other places. I don't have a strong opinion, but it's also used for pointer dereference, so neither Times nor Multiply are very accurate 🤷‍♂️

On a related note, we could also move to the model that just uses the character as the token "name", that would avoid having to name tokens, which is probably the hardest part of making these patches.

I don't feel strongly here fwiw, just a thought that popped in my head. I'm sure one of you will figure out a good name/solution for this :)

Oh, I just realized I also named the operation Times 🤦 I was thinking this was the token. I'll rename before landing

I actually like the "unify all binary operators into a single class, and give that class a char m_operator; model. The different binary operators are not structurally different.

webkit-early-warning-system · 2023-01-24T17:00:10Z

EWS run on previous version of this PR (hash 4d9f215)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
✅ 🧪 webkitpy	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
✅ 🧪 services	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

litherum · 2023-01-25T08:26:04Z

Source/WebGPU/WGSL/AST/ASTBinaryExpression.h

@@ -31,6 +31,7 @@ namespace WGSL::AST {

 enum class BinaryOperation : uint8_t {
    Add,
+    Times,


I actually like the "unify all binary operators into a single class, and give that class a char m_operator; model. The different binary operators are not structurally different.

webkit-early-warning-system · 2023-01-25T10:09:23Z

EWS run on current version of this PR (hash 879623b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
✅ 🧪 webkitpy	✅ 🧪 api-ios	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
~~🛠 🧪 jsc~~	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
~~🛠 🧪 jsc-arm64~~	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
✅ 🧪 services	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
✅ 🛠 🧪 merge	✅ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

https://bugs.webkit.org/show_bug.cgi?id=251088 <rdar://problem/104600637> Reviewed by Myles C. Maxfield and Dean Jackson. As the title says: add parsing and a new BinaryOperation for multiplication. The lexing will have to change as we add support for comments and other operators, but this will do for now and enables testing more complex shaders. * Source/WebGPU/WGSL/AST/ASTBinaryExpression.h: * Source/WebGPU/WGSL/AST/ASTStringDumper.cpp: (WGSL::AST::StringDumper::visit): * Source/WebGPU/WGSL/Lexer.cpp: (WGSL::Lexer<T>::lex): * Source/WebGPU/WGSL/Metal/MetalFunctionWriter.cpp: (WGSL::Metal::FunctionDefinitionWriter::visit): * Source/WebGPU/WGSL/Parser.cpp: (WGSL::Parser<Lexer>::parseMultiplicativeExpression): * Source/WebGPU/WGSL/Token.cpp: (WGSL::toString): * Source/WebGPU/WGSL/Token.h: Canonical link: https://commits.webkit.org/259347@main

webkit-commit-queue · 2023-01-25T11:33:41Z

Committed 259347@main (f10f7a6): https://commits.webkit.org/259347@main

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

tadeuzagallo self-assigned this Jan 24, 2023

tadeuzagallo added the WebGPU For bugs in WebGPU label Jan 24, 2023

tadeuzagallo requested review from litherum, djg and mwyrzykowski January 24, 2023 15:31

nt1m reviewed Jan 24, 2023

View reviewed changes

tadeuzagallo force-pushed the eng/WGSL-Add-support-for-multiplication branch from 2a6b81d to 4d9f215 Compare January 24, 2023 16:38

grorg approved these changes Jan 24, 2023

View reviewed changes

litherum approved these changes Jan 25, 2023

View reviewed changes

tadeuzagallo force-pushed the eng/WGSL-Add-support-for-multiplication branch from 4d9f215 to 879623b Compare January 25, 2023 10:05

tadeuzagallo added the merge-queue Applied to send a pull request to merge-queue label Jan 25, 2023

webkit-early-warning-system force-pushed the eng/WGSL-Add-support-for-multiplication branch from 879623b to f10f7a6 Compare January 25, 2023 11:33

webkit-early-warning-system merged commit f10f7a6 into WebKit:main Jan 25, 2023

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

tadeuzagallo deleted the eng/WGSL-Add-support-for-multiplication branch May 17, 2023 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WGSL] Add support for multiplication #9043

[WGSL] Add support for multiplication #9043

tadeuzagallo commented Jan 24, 2023 •

edited by webkit-early-warning-system

nt1m Jan 24, 2023

tadeuzagallo Jan 24, 2023

tadeuzagallo Jan 24, 2023

nt1m Jan 24, 2023

tadeuzagallo Jan 24, 2023

litherum Jan 25, 2023

webkit-early-warning-system commented Jan 24, 2023 •

edited

litherum Jan 25, 2023

webkit-early-warning-system commented Jan 25, 2023 •

edited

webkit-commit-queue commented Jan 25, 2023

[WGSL] Add support for multiplication #9043

[WGSL] Add support for multiplication #9043

Conversation

tadeuzagallo commented Jan 24, 2023 • edited by webkit-early-warning-system

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 Jan 24, 2023 • edited

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 25, 2023 • edited

webkit-commit-queue commented Jan 25, 2023

tadeuzagallo commented Jan 24, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 24, 2023 •

edited

webkit-early-warning-system commented Jan 25, 2023 •

edited