Bug 237629 - [WGSL] Implement enough of the Parser for the simplest shaders
Summary: [WGSL] Implement enough of the Parser for the simplest shaders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on: 233276 236655
Blocks:
  Show dependency treegraph
 
Reported: 2022-03-08 16:52 PST by Robin Morisset
Modified: 2022-03-09 14:27 PST (History)
2 users (show)

See Also:


Attachments
Patch (56.23 KB, patch)
2022-03-08 16:56 PST, Robin Morisset
mmaxfield: review+
Details | Formatted Diff | Diff
Patch for landing (56.23 KB, patch)
2022-03-09 11:56 PST, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2022-03-08 16:52:21 PST
...
Comment 1 Robin Morisset 2022-03-08 16:56:58 PST
Created attachment 454175 [details]
Patch
Comment 2 Myles C. Maxfield 2022-03-08 21:41:50 PST
Comment on attachment 454175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454175&action=review

> Source/WebGPU/WGSL/CompilationMessage.h:57
> +using Warning = CompilationMessage;
> +using Error = CompilationMessage;

Cute.

> Source/WebGPU/WGSL/Parser.cpp:45
> +#define START_PARSE() \

Is this style of using #defines copied from anywhere?

> Source/WebGPU/WGSL/Parser.cpp:168
> +Expected<AST::ShaderModule, Error> parse(const String& wgsl)
> +{
> +    Lexer lexer(wgsl);
> +    Parser parser(lexer);
> +    
> +    return parser.parseShader();
> +}

Did you consider generating this code from a grammar file?

> Source/WebGPU/WGSL/Parser.cpp:511
> +    START_PARSE();

Is there a way to guarantee that parsing functions always start with a START_PARSE() macro?
Comment 3 Robin Morisset 2022-03-09 11:43:04 PST
Thanks for the review.

(In reply to Myles C. Maxfield from comment #2)
> Comment on attachment 454175 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454175&action=review
> 
> > Source/WebGPU/WGSL/CompilationMessage.h:57
> > +using Warning = CompilationMessage;
> > +using Error = CompilationMessage;
> 
> Cute.

Thanks, I found it makes function signatures more readable.

> > Source/WebGPU/WGSL/Parser.cpp:45
> > +#define START_PARSE() \
> 
> Is this style of using #defines copied from anywhere?

The exact macros are different, but both JavaScriptCore/parser/Parser.cpp and the old WHLSL parser (which I used as inspiration) use a ton of macros as well. Regular C++ is just not the most pleasant language for writing this kind of code.

> > Source/WebGPU/WGSL/Parser.cpp:168
> > +Expected<AST::ShaderModule, Error> parse(const String& wgsl)
> > +{
> > +    Lexer lexer(wgsl);
> > +    Parser parser(lexer);
> > +    
> > +    return parser.parseShader();
> > +}
> 
> Did you consider generating this code from a grammar file?

Yes, it would have been easier, but could have been an issue if the language later becomes more complicated. It would also have put a pretty low cap on the quality of the error messages that is achievable. As a result parser generators are rarely used for the major compilers for real-world languages.

> 
> > Source/WebGPU/WGSL/Parser.cpp:511
> > +    START_PARSE();
> 
> Is there a way to guarantee that parsing functions always start with a
> START_PARSE() macro?

Unfortunately I don't know of such a way. But forgetting the macro just gives you an error message as `_startOfElementPosition` is not defined.
Comment 4 Robin Morisset 2022-03-09 11:56:41 PST
Created attachment 454275 [details]
Patch for landing

Just fixed style nits.
Comment 5 EWS 2022-03-09 14:26:32 PST
Committed r291075 (248241@main): <https://commits.webkit.org/248241@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454275 [details].
Comment 6 Radar WebKit Bug Importer 2022-03-09 14:27:21 PST
<rdar://problem/90057433>