Bug 236655 – [WGSL] Add enough of the AST for the simplest shaders

• Component: WebGPU
• Status: RESOLVED FIXED

[Robin Morisset]

The parser will come in its own patch.

[Robin Morisset]

Created attachment 452069 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 452069 [details]
Patch

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

r=me, but please consider the LFC approach. Also please don't land until EWS says it can build.

> Source/WebGPU/WGSL/AST/Attribute.h:35
> +class Attribute : public ASTNode

Attributes are ASTNodes? Why?

> Source/WebGPU/WGSL/AST/Expression.h:60
> +    bool isBoolLiteral() const { return kind() == Kind::BoolLiteral; }
> +    bool isInt32Literal() const { return kind() == Kind::Int32Literal; }
> +    bool isUInt32Literal() const { return kind() == Kind::Uint32Literal; }
> +    bool isFloat32Literal() const { return kind() == Kind::Float32Literal; }
> +    bool isIdentifier() const { return kind() == Kind::Identifier; }
> +    bool isStructureAccess() const { return kind() == Kind::StructureAccess; }
> +    bool isTypeConversion() const { return kind() == Kind::TypeConversion; }

We usually use virtual functions for these instead of an explicit m_kind. What's the reason you prefer to spend the memory?

> Source/WebGPU/WGSL/AST/FunctionDecl.h:51
> +                    return { CompilationMessage("It is not valid to apply the builtin attribute twice to a parameter", attr->span()) };

Do you really want to do validation checking inside the AST nodes?

LFC uses a design that clearly separates the data model from the controller that operates on that data model. It uses things like explicit Builder classes rather than calling methods on the AST nodes themselves. I think that's a better design, because 1) It makes it clear from types involved which operation is being performed in which stage in the compiler, so it's always clear where invariants hold and when it's legal to call certain functions 2) it makes code easier to read (and, I would claim, write) by having each file narrowly contain a focused set of methods, rather than a sprawl of whatever anything might want to do on an ASTNode 3) By making stronger separation of functionality, we can more easily reason about the code at the macro level, for example, to some day try to run some parts of the compiler in parallel (which may not be that useful for a WGSL compiler, but has seen some amazing benefits in LFC).

> Source/WebGPU/WGSL/AST/Shader.h:37
> +class Shader : ASTNode {

I would call this ShaderModule. Conceptually I think of "shaders" as individual entry points.

> Source/WebGPU/WGSL/AST/VariableQualifier.h:46
> +class VariableQualifier : public ASTNode {

Ditto about not understanding why this is an ASTNode and not just a member field of a variable

[Robin Morisset]

The build issue was just a missing file.

> > Source/WebGPU/WGSL/AST/Attribute.h:35
> > +class Attribute : public ASTNode
> 
> Attributes are ASTNodes? Why?

So that they have a SourceSpan, so that we can give precise error messages in case of repeated/invalid attributes.

> > Source/WebGPU/WGSL/AST/Expression.h:60
> > +    bool isBoolLiteral() const { return kind() == Kind::BoolLiteral; }
> > +    bool isInt32Literal() const { return kind() == Kind::Int32Literal; }
> > +    bool isUInt32Literal() const { return kind() == Kind::Uint32Literal; }
> > +    bool isFloat32Literal() const { return kind() == Kind::Float32Literal; }
> > +    bool isIdentifier() const { return kind() == Kind::Identifier; }
> > +    bool isStructureAccess() const { return kind() == Kind::StructureAccess; }
> > +    bool isTypeConversion() const { return kind() == Kind::TypeConversion; }
> 
> We usually use virtual functions for these instead of an explicit m_kind.
> What's the reason you prefer to spend the memory?

I followed the examples I found in the WebCore code, e.g. StyleRuleBase.

It is actually a possible memory saving, since we are only using an 8-bit m_kind, instead of a 64-bit virtual table pointer.

But I am fully willing to move to virtual methods, it is simpler and would match for example AccessibilityObject.

> > Source/WebGPU/WGSL/AST/FunctionDecl.h:51
> > +                    return { CompilationMessage("It is not valid to apply the builtin attribute twice to a parameter", attr->span()) };
> 
> Do you really want to do validation checking inside the AST nodes?
> 
> LFC uses a design that clearly separates the data model from the controller
> that operates on that data model. It uses things like explicit Builder
> classes rather than calling methods on the AST nodes themselves. I think
> that's a better design, because 1) It makes it clear from types involved
> which operation is being performed in which stage in the compiler, so it's
> always clear where invariants hold and when it's legal to call certain
> functions 2) it makes code easier to read (and, I would claim, write) by
> having each file narrowly contain a focused set of methods, rather than a
> sprawl of whatever anything might want to do on an ASTNode 3) By making
> stronger separation of functionality, we can more easily reason about the
> code at the macro level, for example, to some day try to run some parts of
> the compiler in parallel (which may not be that useful for a WGSL compiler,
> but has seen some amazing benefits in LFC).

I am rewriting this to follow your suggestion.

> > Source/WebGPU/WGSL/AST/Shader.h:37
> > +class Shader : ASTNode {
> 
> I would call this ShaderModule. Conceptually I think of "shaders" as
> individual entry points.

Ok, renamed.

> > Source/WebGPU/WGSL/AST/VariableQualifier.h:46
> > +class VariableQualifier : public ASTNode {
> 
> Ditto about not understanding why this is an ASTNode and not just a member
> field of a variable

We may want to give precise error messages about its location.

[Robin Morisset]

Created attachment 452359 [details]
Patch

[Sam Weinig]

Comment on attachment 452359 [details]
Patch

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

> Source/WebGPU/WGSL/AST/Attribute.h:30
> +#include "wtf/UniqueRef.h"
> +#include "wtf/Vector.h"

wtf #includes should use #include <<> syntax since they are not part of this project.

> Source/WebGPU/WGSL/AST/Attribute.h:36
> +class Attribute : public ASTNode
> +{

{ should go on the previous line. (There are few more like this that need updating in this file and others in the patch).

> Source/WebGPU/WGSL/AST/Expressions/IdentifierExpression.h:33
> +class IdentifierExpression : public Expression {

Unclear to me if this is a final class, but if there is no intention of subclassing this, you should add the "final" modifier. (same for other classes like this).

> Source/WebGPU/WGSL/AST/GlobalDirective.h:39
> +    StringView m_name;

Can this ever be initialized?

[Robin Morisset]

Created attachment 452379 [details]
Patch

- Moved kind() methods from using a field to being virtual.
- Added missing TypeDecls
- Fixed style nits
- Applied Sam's suggestions

[Myles C. Maxfield]

Comment on attachment 452379 [details]
Patch

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

> Source/WebGPU/WGSL/AST/TypeDecl.h:74
> +        Vec2,

Surprising to see this rather than a pointer to an inner type.

[Radar WebKit Bug Importer]

<rdar://problem/89307070>

[Robin Morisset]

Created attachment 454049 [details]
Patch for landing

Just rebased

[EWS]

Committed r291005 (248181@main): <https://commits.webkit.org/248181@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454049 [details].