Bug 175925 – Initial reference implementation scaffolding for WebGPU Shading Language prototype

• Component: WebGPU
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 318967 [details]
it's a start

[Keith Miller]

Comment on attachment 318967 [details]
it's a start

Cool, I think we should consider using ES6 modules for this though. It will be harder to change to later if we don't start with it now and it will help us enforce some amount of abstraction boundaries.

[Filip Pizlo]

Created attachment 319008 [details]
a little more

Nothing more fun than a Thursday morning hacking spree.

[Filip Pizlo]

Created attachment 319056 [details]
more

[Filip Pizlo]

Fixed component.

[Filip Pizlo]

Created attachment 319065 [details]
core type checker is looking good

[Filip Pizlo]

Created attachment 319100 [details]
steadily adding expressions

[Filip Pizlo]

Created attachment 319116 [details]
a bit more things

The AST is rich enough to say things like:

int32 foo(int32 x)
{
    return x + 42;
}

But we cannot parse this yet.

Also, I haven't even tried running it.

[Filip Pizlo]

Created attachment 319122 [details]
started writing the parser

[Filip Pizlo]

Created attachment 319136 [details]
a bit more

Working through the parser.  Almost at the point where it can parse interesting things.

[Filip Pizlo]

Created attachment 319140 [details]
parser is getting real

[Filip Pizlo]

Created attachment 319143 [details]
lot more code in the parser

The parser is so dense.  It's not done yet.

[Filip Pizlo]

Created attachment 319147 [details]
more parser

[Filip Pizlo]

Created attachment 319162 [details]
more

I'm mostly done with the parser.

I'm also starting to notice some weirdnesses in the type system regarding arrays.

Overall it's looking pretty good.

[Filip Pizlo]

Created attachment 319205 [details]
added more overload resolution machinery

[Filip Pizlo]

Created attachment 319220 [details]
started working on the interrpeter

[Filip Pizlo]

Created attachment 319226 [details]
wrote an inliner

That let me do the hash-cons type instantiation, which allows me to allocate LValues.

[Filip Pizlo]

Created attachment 319229 [details]
more

[Filip Pizlo]

Created attachment 319244 [details]
inching forward with native functions

[Filip Pizlo]

Created attachment 319256 [details]
it is written

Now I need to test it.

[Filip Pizlo]

Created attachment 319259 [details]
starting to debug

[Filip Pizlo]

Created attachment 319268 [details]
getting close to parsing the first test

[Filip Pizlo]

Created attachment 319284 [details]
the patch

[Joseph Pecoraro]

Comment on attachment 319284 [details]
the patch

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

Using Source/WebInspectorUI/.eslintrc with ESLint I trimmed down to a list of things that look like legit issues that would cause unexpected behavior or runtime exceptions and a few pedantic things like missing semicolons. Probably would be worth addressing these early otherwise they will just have to be debugged and addressed later when hooking up sections of the code:

    Tools/ArrayLangRI/ArrayType.js
      28:5   error  Expected to call 'super()'              constructor-super
      30:9   error  'this' is not allowed before 'super()'  no-this-before-super
      31:9   error  'this' is not allowed before 'super()'  no-this-before-super
      32:9   error  'this' is not allowed before 'super()'  no-this-before-super

    Tools/ArrayLangRI/Block.js
      48:51  error  Expected an assignment or function call and instead saw an expression  no-unused-expressions
      48:51  error  Unreachable code                                                       no-unreachable

    Tools/ArrayLangRI/ConstexprTypeParameter.js
      28:5   error  Expected to call 'super()'                          constructor-super
      30:9   error  'this' is not allowed before 'super()'              no-this-before-super
      31:9   error  'this' is not allowed before 'super()'              no-this-before-super
      32:9   error  'this' is not allowed before 'super()'              no-this-before-super

    Tools/ArrayLangRI/FuncInstantiator.js
      61:13  error  'parameters' is not defined                   no-undef
      62:13  error  'body' is not defined                         no-undef
  
    Tools/ArrayLangRI/FunctionLikeBlock.js
      28:5   error  Expected to call 'super()'                     constructor-super
      30:9   error  'this' is not allowed before 'super()'         no-this-before-super
      31:9   error  'this' is not allowed before 'super()'         no-this-before-super
      32:9   error  'this' is not allowed before 'super()'         no-this-before-super
      33:9   error  'this' is not allowed before 'super()'         no-this-before-super
      43:26  error  'parameters' is not defined                    no-undef
      43:48  error  'block' is not defined                         no-undef
      43:64  error  'argumentList' is not defined                  no-undef

    Tools/ArrayLangRI/NameContext.js
      158:18  error  'map' is assigned a value but never used  no-unused-vars
      - Yeah this does seem peculiar but it might not be wrong.

    Tools/ArrayLangRI/NameResolver.js
      132:37  error  'node' is not defined                     no-undef
      141:39  error  'node' is not defined                     no-undef
      157:35  error  'origin' is not defined                   no-undef
      163:39  error  'origin' is not defined                   no-undef
      165:39  error  'origin' is not defined                   no-undef

    Tools/ArrayLangRI/NativeTypeInstance.js
      28:5   error  Expected to call 'super()'                      constructor-super
      30:9   error  'this' is not allowed before 'super()'          no-this-before-super
      31:9   error  'this' is not allowed before 'super()'          no-this-before-super

    Tools/ArrayLangRI/Parse.js
      223:43  error  Missing semicolon                                                semi
      239:13  error  Expected a conditional expression and instead saw an assignment  no-cond-assign
      475:60  error  'term' is not defined                                            no-undef
      544:48  error  Missing semicolon                                                semi

    Tools/ArrayLangRI/Protocol.js
      28:5   error  Expected to call 'super()'              constructor-super
      30:9   error  'this' is not allowed before 'super()'  no-this-before-super
      31:9   error  'this' is not allowed before 'super()'  no-this-before-super

    Tools/ArrayLangRI/ProtocolDecl.js
       30:28  error  Missing semicolon                                       semi
       78:21  error  'signature' is not defined                              no-undef
       88:18  error  'abstractSignature' is assigned a value but never used  no-unused-vars

    Tools/ArrayLangRI/PtrType.js
      30:13  error  Unexpected negating the left operand of 'instanceof' operator  no-unsafe-negation

    Tools/ArrayLangRI/Rewriter.js
       51:53  error  'typeParameter' is not defined           no-undef
       - This one looks like a typo

    Tools/ArrayLangRI/StructType.js
      28:5   error  Expected to call 'super()'              constructor-super
      30:9   error  'this' is not allowed before 'super()'  no-this-before-super
      31:9   error  'this' is not allowed before 'super()'  no-this-before-super
      32:9   error  'this' is not allowed before 'super()'  no-this-before-super
      33:9   error  'this' is not allowed before 'super()'  no-this-before-super

    Tools/ArrayLangRI/TypeOrVariableRef.js
      28:5   error  Expected to call 'super()'                     constructor-super
      30:9   error  'this' is not allowed before 'super()'         no-this-before-super
      31:9   error  'this' is not allowed before 'super()'         no-this-before-super

    Tools/ArrayLangRI/TypeRef.js
      47:32  error  'type' is not defined                no-undef
      47:39  error  'type' is not defined                no-undef

    Tools/ArrayLangRI/TypeVariable.js
      28:5   error  Expected to call 'super()'                constructor-super
      30:9   error  'this' is not allowed before 'super()'    no-this-before-super
      31:9   error  'this' is not allowed before 'super()'    no-this-before-super
      32:9   error  'this' is not allowed before 'super()'    no-this-before-super

    Tools/ArrayLangRI/Visitor.js
       89:18  error  'typeParamter' is assigned a value but never used  no-unused-vars
       90:13  error  'typeParameter' is not defined                     no-undef

> Tools/ArrayLangRI/ALSyntaxError.js:23
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 

The trailing whitespace in these copyright comments is like a small virus that spreads throughout our codebase =)

> Tools/ArrayLangRI/AddressSpace.js:27
> +const addressSpaces = ["constant", "device", "threadgroup", "thread"];

In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually.

> Tools/ArrayLangRI/Block.js:48
> +        return "{ " + this.statements.join("; "); + "; }";

The semicolon after the join breaks the intended behavior here.

> Tools/ArrayLangRI/CommaExpression.js:44
> +};

ESLint had some warnings about these as unnecessary semicolons. You have them sometimes and sometimes you don't. Up to you if you want to clean them up.

> Tools/ArrayLangRI/FuncSignature.js:26
> +"use strict";
> +

This file is empty! Drop it?

> Tools/ArrayLangRI/NameResolver.js:34
> +    // It's totally OK to instantiate this with zero arguments (i.e. passing undefined for both of
> +    // them) if you're creating a Checker to visit a Program.

This alludes to multiple arguments but that is no longer the case. Seems the comment is stale.

> Tools/ArrayLangRI/ProtocolDecl.js:40
> +        // FIXME: Check this! (OOPS!)

Heh, should this be tweaked now or later?

[Myles C. Maxfield]

Comment on attachment 319284 [details]
the patch

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

Partial review, not done.

> Tools/ChangeLog:3
> +        Initial reference implementation scaffolding for ArrayLang

WebGPU Shading Language

> Tools/ChangeLog:9
> +        experimental shader language we are calling ArrayLang for now.

Ditto.

> Tools/ChangeLog:17
> +        protocol Addable { Addable operator+(Addable, Addable); }

And "int operator+(int, int);"

> Tools/ChangeLog:24
> +        Each variable behaves as if it was declared "static", with one copy per type instantiation.

Using C++'s notion of "static" variables

>> Tools/ArrayLangRI/AddressSpace.js:27
>> +const addressSpaces = ["constant", "device", "threadgroup", "thread"];
> 
> In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually.

We don't want to run this in a browser eventually. This is a proof-of-concept.

> Tools/ArrayLangRI/ArrayRefType.js:36
> +            if (this.addressSpace != "thread")
> +                return false;

This looks like a great jumping-off point for me.

[Filip Pizlo]

(In reply to Myles C. Maxfield from comment #25)
> Comment on attachment 319284 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319284&action=review
> 
> Partial review, not done.
> 
> > Tools/ChangeLog:3
> > +        Initial reference implementation scaffolding for ArrayLang
> 
> WebGPU Shading Language
> 
> > Tools/ChangeLog:9
> > +        experimental shader language we are calling ArrayLang for now.
> 
> Ditto.

Let’s come up with a shorter name, or use ArrayLang in the initial commit. Renaming is easy. 

> 
> > Tools/ChangeLog:17
> > +        protocol Addable { Addable operator+(Addable, Addable); }
> 
> And "int operator+(int, int);"

Not sure what you mean. That operator is provided by the system. I’m saying that the user could write an Addable protocol, as an example of what the typesyatem is capable of. 

> 
> > Tools/ChangeLog:24
> > +        Each variable behaves as if it was declared "static", with one copy per type instantiation.
> 
> Using C++'s notion of "static" variables
> 
> >> Tools/ArrayLangRI/AddressSpace.js:27
> >> +const addressSpaces = ["constant", "device", "threadgroup", "thread"];
> > 
> > In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually.
> 
> We don't want to run this in a browser eventually. This is a
> proof-of-concept.

This reference implementation will run in a browser in the sense that we will make it a page you can navigate to. 

> 
> > Tools/ArrayLangRI/ArrayRefType.js:36
> > +            if (this.addressSpace != "thread")
> > +                return false;
> 
> This looks like a great jumping-off point for me.

[Joseph Pecoraro]

> > > In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually.
> > 
> > We don't want to run this in a browser eventually. This is a proof-of-concept.
> 
> This reference implementation will run in a browser in the sense that we
> will make it a page you can navigate to. 

I just tested this and a global const variable defined in one <script> was accessible in another <script>. So it appears my comment is incorrect (or perhaps just outdated).

I do know we had to make a change to Web Inspector code for exactly this situation in the past:
https://trac.webkit.org/changeset/187012/webkit#file82

However, many things could have changed with global `const` behavior between then (the original introduction) and now!

[Saam Barati]

(In reply to Joseph Pecoraro from comment #27)
> > > > In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually.
> > > 
> > > We don't want to run this in a browser eventually. This is a proof-of-concept.
> > 
> > This reference implementation will run in a browser in the sense that we
> > will make it a page you can navigate to. 
> 
> I just tested this and a global const variable defined in one <script> was
> accessible in another <script>. So it appears my comment is incorrect (or
> perhaps just outdated).
Yeah this is the specced behavior.
> 
> I do know we had to make a change to Web Inspector code for exactly this
> situation in the past:
> https://trac.webkit.org/changeset/187012/webkit#file82
> 
> However, many things could have changed with global `const` behavior between
> then (the original introduction) and now!
Yeah Yue behavior did change. I originally implemented it incorrectly, which had let/const visible only per script. I later fixed it by making top level let/const visible across scripts.

[Myles C. Maxfield]

Comment on attachment 319284 [details]
the patch

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

At a high level, this seems fine. I really appreciate the current design's primary goal being clarity. I'd like to take this and run with it; there are lots of ideas I have for what we can do next with it. It doesn't seem that being extremely detail-oriented would be valuable for this first patch, as this is intended to be platform for further evolution of the language. There is a chance that some piece of this code will end up in production, but that chance is slim.

> Tools/ArrayLangRI/EBufferBuilder.js:33
> +        // NOTE: I don't think that this is necessary.

FIXME?

[Keith Miller]

Comment on attachment 319284 [details]
the patch

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

It's Keith's beddy-bye time, I'm tentatively r-ing based on my comments. I'll read more details tomorrow to make sure I understand correctly.

>>> Tools/ChangeLog:3
>>> +        Initial reference implementation scaffolding for ArrayLang
>> 
>> WebGPU Shading Language
> 
> Let’s come up with a shorter name, or use ArrayLang in the initial commit. Renaming is easy.

Why not WebSL? Are there any shader languages that wouldn't be for the GPU?

>>> Tools/ChangeLog:24
>>> +        Each variable behaves as if it was declared "static", with one copy per type instantiation.
>> 
>> Using C++'s notion of "static" variables
> 
> This reference implementation will run in a browser in the sense that we will make it a page you can navigate to.

I don't like this semantics, unless it's a compile error to use an uninitialized value. In fact, I'll r- on this point alone. e.g.

int foo() {
    int a;
    return a++;
}

would return a different value on each invocation.

>> Tools/ArrayLangRI/ALSyntaxError.js:23
>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> 
> The trailing whitespace in these copyright comments is like a small virus that spreads throughout our codebase =)

Indeed, pizlo needs to install https://github.com/lewang/ws-butler. I recommended it to him previously but he chose to ignore me :(

>>> Tools/ArrayLangRI/AddressSpace.js:27
>>> +const addressSpaces = ["constant", "device", "threadgroup", "thread"];
>> 
>> In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually.
> 
> We don't want to run this in a browser eventually. This is a proof-of-concept.

Unless I have misunderstood you Myles, I think we do want this to run in a browser. My understanding is this would be more or less a reference implementation.

[Filip Pizlo]

(In reply to Keith Miller from comment #30)
> Comment on attachment 319284 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319284&action=review
> 
> It's Keith's beddy-bye time, I'm tentatively r-ing based on my comments.
> I'll read more details tomorrow to make sure I understand correctly.

Can you please undo that?  The purpose of landing this is so that someone other than me can make changes. We are blocked on this landing. Using r- to indicate that you don’t understand the semantics is not really great since this is a super rough draft. I would have advocated landing as is even if I did not yet have a story for how variables get initialized. It’s great if stuff like that happens after this lands so that I’m on the only one working on it. 

> 
> >>> Tools/ChangeLog:3
> >>> +        Initial reference implementation scaffolding for ArrayLang
> >> 
> >> WebGPU Shading Language
> > 
> > Let’s come up with a shorter name, or use ArrayLang in the initial commit. Renaming is easy.
> 
> Why not WebSL? Are there any shader languages that wouldn't be for the GPU?
> 
> >>> Tools/ChangeLog:24
> >>> +        Each variable behaves as if it was declared "static", with one copy per type instantiation.
> >> 
> >> Using C++'s notion of "static" variables
> > 
> > This reference implementation will run in a browser in the sense that we will make it a page you can navigate to.
> 
> I don't like this semantics, unless it's a compile error to use an
> uninitialized value. 

There are no uniniyialized values. Variables get initialized to the default value for the type at the moment the variable declaration executes. 

> In fact, I'll r- on this point alone. e.g.
> 
> int foo() {
>     int a;
>     return a++;
> }
> 
> would return a different value on each invocation.

This always returns zero. 

> 
> >> Tools/ArrayLangRI/ALSyntaxError.js:23
> >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> > 
> > The trailing whitespace in these copyright comments is like a small virus that spreads throughout our codebase =)
> 
> Indeed, pizlo needs to install https://github.com/lewang/ws-butler. I
> recommended it to him previously but he chose to ignore me :(
> 
> >>> Tools/ArrayLangRI/AddressSpace.js:27
> >>> +const addressSpaces = ["constant", "device", "threadgroup", "thread"];
> >> 
> >> In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually.
> > 
> > We don't want to run this in a browser eventually. This is a proof-of-concept.
> 
> Unless I have misunderstood you Myles, I think we do want this to run in a
> browser. My understanding is this would be more or less a reference
> implementation.

[Keith Miller]

(In reply to Filip Pizlo from comment #31)
> (In reply to Keith Miller from comment #30)
> > Comment on attachment 319284 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=319284&action=review
> > 
> > It's Keith's beddy-bye time, I'm tentatively r-ing based on my comments.
> > I'll read more details tomorrow to make sure I understand correctly.
> 
> Can you please undo that?  The purpose of landing this is so that someone
> other than me can make changes. We are blocked on this landing. Using r- to
> indicate that you don’t understand the semantics is not really great since
> this is a super rough draft. I would have advocated landing as is even if I
> did not yet have a story for how variables get initialized. It’s great if
> stuff like that happens after this lands so that I’m on the only one working
> on it. 
> 

That's a fair point. My goal was to make my objection adamantly clear. If the patch landed in it's "old" state (I use quotes because it appears I was mistaken), I was concerned that the language might proceed with the "old" semantics. Such a language decision would have been a disaster in my opinion.

> > 
> > >>> Tools/ChangeLog:3
> > >>> +        Initial reference implementation scaffolding for ArrayLang
> > >> 
> > >> WebGPU Shading Language
> > > 
> > > Let’s come up with a shorter name, or use ArrayLang in the initial commit. Renaming is easy.
> > 
> > Why not WebSL? Are there any shader languages that wouldn't be for the GPU?
> > 
> > >>> Tools/ChangeLog:24
> > >>> +        Each variable behaves as if it was declared "static", with one copy per type instantiation.
> > >> 
> > >> Using C++'s notion of "static" variables
> > > 
> > > This reference implementation will run in a browser in the sense that we will make it a page you can navigate to.
> > 
> > I don't like this semantics, unless it's a compile error to use an
> > uninitialized value. 
> 
> There are no uniniyialized values. Variables get initialized to the default
> value for the type at the moment the variable declaration executes.

I see, I misunderstood your statement as a statement about how the language would be specified rather than an implementation detail of the interpreter.
 
> 
> > In fact, I'll r- on this point alone. e.g.
> > 
> > int foo() {
> >     int a;
> >     return a++;
> > }
> > 
> > would return a different value on each invocation.
> 
> This always returns zero. 
> 
> > 
> > >> Tools/ArrayLangRI/ALSyntaxError.js:23
> > >> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> > > 
> > > The trailing whitespace in these copyright comments is like a small virus that spreads throughout our codebase =)
> > 
> > Indeed, pizlo needs to install https://github.com/lewang/ws-butler. I
> > recommended it to him previously but he chose to ignore me :(
> > 
> > >>> Tools/ArrayLangRI/AddressSpace.js:27
> > >>> +const addressSpaces = ["constant", "device", "threadgroup", "thread"];
> > >> 
> > >> In a browser, I think the the lexical nature of the `const` would prevent this constant from being used outside of this file. Same with the cases of just `class Foo { ... };`. In your case I think it all works in because All.js just inline `load(...)`s all the individual files. You don't need to change, just something to consider if you want to try running this in a browser eventually.
> > > 
> > > We don't want to run this in a browser eventually. This is a proof-of-concept.
> > 
> > Unless I have misunderstood you Myles, I think we do want this to run in a
> > browser. My understanding is this would be more or less a reference
> > implementation.

[Filip Pizlo]

(In reply to Joseph Pecoraro from comment #24)
> Comment on attachment 319284 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319284&action=review
> 
> Using Source/WebInspectorUI/.eslintrc with ESLint I trimmed down to a list
> of things that look like legit issues that would cause unexpected behavior
> or runtime exceptions and a few pedantic things like missing semicolons.
> Probably would be worth addressing these early otherwise they will just have
> to be debugged and addressed later when hooking up sections of the code:
> 
>     Tools/ArrayLangRI/ArrayType.js
>       28:5   error  Expected to call 'super()'              constructor-super
>       30:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
>       31:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
>       32:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
> 
>     Tools/ArrayLangRI/Block.js
>       48:51  error  Expected an assignment or function call and instead saw
> an expression  no-unused-expressions
>       48:51  error  Unreachable code                                        
> no-unreachable
> 
>     Tools/ArrayLangRI/ConstexprTypeParameter.js
>       28:5   error  Expected to call 'super()'                         
> constructor-super
>       30:9   error  'this' is not allowed before 'super()'             
> no-this-before-super
>       31:9   error  'this' is not allowed before 'super()'             
> no-this-before-super
>       32:9   error  'this' is not allowed before 'super()'             
> no-this-before-super
> 
>     Tools/ArrayLangRI/FuncInstantiator.js
>       61:13  error  'parameters' is not defined                   no-undef
>       62:13  error  'body' is not defined                         no-undef
>   
>     Tools/ArrayLangRI/FunctionLikeBlock.js
>       28:5   error  Expected to call 'super()'                    
> constructor-super
>       30:9   error  'this' is not allowed before 'super()'        
> no-this-before-super
>       31:9   error  'this' is not allowed before 'super()'        
> no-this-before-super
>       32:9   error  'this' is not allowed before 'super()'        
> no-this-before-super
>       33:9   error  'this' is not allowed before 'super()'        
> no-this-before-super
>       43:26  error  'parameters' is not defined                    no-undef
>       43:48  error  'block' is not defined                         no-undef
>       43:64  error  'argumentList' is not defined                  no-undef
> 
>     Tools/ArrayLangRI/NameContext.js
>       158:18  error  'map' is assigned a value but never used  no-unused-vars
>       - Yeah this does seem peculiar but it might not be wrong.
> 
>     Tools/ArrayLangRI/NameResolver.js
>       132:37  error  'node' is not defined                     no-undef
>       141:39  error  'node' is not defined                     no-undef
>       157:35  error  'origin' is not defined                   no-undef
>       163:39  error  'origin' is not defined                   no-undef
>       165:39  error  'origin' is not defined                   no-undef
> 
>     Tools/ArrayLangRI/NativeTypeInstance.js
>       28:5   error  Expected to call 'super()'                     
> constructor-super
>       30:9   error  'this' is not allowed before 'super()'         
> no-this-before-super
>       31:9   error  'this' is not allowed before 'super()'         
> no-this-before-super
> 
>     Tools/ArrayLangRI/Parse.js
>       223:43  error  Missing semicolon                                      
> semi
>       239:13  error  Expected a conditional expression and instead saw an
> assignment  no-cond-assign
>       475:60  error  'term' is not defined                                  
> no-undef
>       544:48  error  Missing semicolon                                      
> semi
> 
>     Tools/ArrayLangRI/Protocol.js
>       28:5   error  Expected to call 'super()'              constructor-super
>       30:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
>       31:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
> 
>     Tools/ArrayLangRI/ProtocolDecl.js
>        30:28  error  Missing semicolon                                      
> semi
>        78:21  error  'signature' is not defined                             
> no-undef
>        88:18  error  'abstractSignature' is assigned a value but never used 
> no-unused-vars
> 
>     Tools/ArrayLangRI/PtrType.js
>       30:13  error  Unexpected negating the left operand of 'instanceof'
> operator  no-unsafe-negation
> 
>     Tools/ArrayLangRI/Rewriter.js
>        51:53  error  'typeParameter' is not defined           no-undef
>        - This one looks like a typo
> 
>     Tools/ArrayLangRI/StructType.js
>       28:5   error  Expected to call 'super()'              constructor-super
>       30:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
>       31:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
>       32:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
>       33:9   error  'this' is not allowed before 'super()' 
> no-this-before-super
> 
>     Tools/ArrayLangRI/TypeOrVariableRef.js
>       28:5   error  Expected to call 'super()'                    
> constructor-super
>       30:9   error  'this' is not allowed before 'super()'        
> no-this-before-super
>       31:9   error  'this' is not allowed before 'super()'        
> no-this-before-super
> 
>     Tools/ArrayLangRI/TypeRef.js
>       47:32  error  'type' is not defined                no-undef
>       47:39  error  'type' is not defined                no-undef
> 
>     Tools/ArrayLangRI/TypeVariable.js
>       28:5   error  Expected to call 'super()'               
> constructor-super
>       30:9   error  'this' is not allowed before 'super()'   
> no-this-before-super
>       31:9   error  'this' is not allowed before 'super()'   
> no-this-before-super
>       32:9   error  'this' is not allowed before 'super()'   
> no-this-before-super
> 
>     Tools/ArrayLangRI/Visitor.js
>        89:18  error  'typeParamter' is assigned a value but never used 
> no-unused-vars
>        90:13  error  'typeParameter' is not defined                    
> no-undef

I fixed almost all of these.

I think that this is OK, so I left it:

if (token = parseThings())
    doThings(token);

> 
> > Tools/ArrayLangRI/ALSyntaxError.js:23
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> 
> The trailing whitespace in these copyright comments is like a small virus
> that spreads throughout our codebase =)
> 
> > Tools/ArrayLangRI/AddressSpace.js:27
> > +const addressSpaces = ["constant", "device", "threadgroup", "thread"];
> 
> In a browser, I think the the lexical nature of the `const` would prevent
> this constant from being used outside of this file. Same with the cases of
> just `class Foo { ... };`. In your case I think it all works in because
> All.js just inline `load(...)`s all the individual files. You don't need to
> change, just something to consider if you want to try running this in a
> browser eventually.
> 
> > Tools/ArrayLangRI/Block.js:48
> > +        return "{ " + this.statements.join("; "); + "; }";
> 
> The semicolon after the join breaks the intended behavior here.

Yeah!  Ha!  I fixed it.

> 
> > Tools/ArrayLangRI/CommaExpression.js:44
> > +};
> 
> ESLint had some warnings about these as unnecessary semicolons. You have
> them sometimes and sometimes you don't. Up to you if you want to clean them
> up.

Ooops.

> 
> > Tools/ArrayLangRI/FuncSignature.js:26
> > +"use strict";
> > +
> 
> This file is empty! Drop it?

Yeah.  I dropped it.

> 
> > Tools/ArrayLangRI/NameResolver.js:34
> > +    // It's totally OK to instantiate this with zero arguments (i.e. passing undefined for both of
> > +    // them) if you're creating a Checker to visit a Program.
> 
> This alludes to multiple arguments but that is no longer the case. Seems the
> comment is stale.

It's a valid comment, I just needed to change "Checker" to "NameResolver".

> 
> > Tools/ArrayLangRI/ProtocolDecl.js:40
> > +        // FIXME: Check this! (OOPS!)
> 
> Heh, should this be tweaked now or later?

I already implemented that in Checker!  I removed the FIXME.

[Filip Pizlo]

(In reply to Myles C. Maxfield from comment #29)
> Comment on attachment 319284 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319284&action=review
> 
> At a high level, this seems fine. I really appreciate the current design's
> primary goal being clarity. I'd like to take this and run with it; there are
> lots of ideas I have for what we can do next with it. It doesn't seem that
> being extremely detail-oriented would be valuable for this first patch, as
> this is intended to be platform for further evolution of the language. There
> is a chance that some piece of this code will end up in production, but that
> chance is slim.
> 
> > Tools/ArrayLangRI/EBufferBuilder.js:33
> > +        // NOTE: I don't think that this is necessary.
> 
> FIXME?

Yup, I removed it.  It wasn't necessary.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/221380/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34165454>

[Filip Pizlo]

(In reply to Keith Miller from comment #2)
> Comment on attachment 318967 [details]
> it's a start
> 
> Cool, I think we should consider using ES6 modules for this though. It will
> be harder to change to later if we don't start with it now and it will help
> us enforce some amount of abstraction boundaries.

Now that it's checked in, is that something that would be easy for you to do?

[Joseph Pecoraro]

> I think that this is OK, so I left it:
> 
> if (token = parseThings())
>     doThings(token);

Oops, yeah I meant to cull those particular warnings and must have missed one.

Thanks for addressing the rest!

[Myles C. Maxfield]

Migrated to https://github.com/gpuweb/WHLSL/issues/12