[webkit-reviews] review denied: [Bug 175925] Initial reference implementation scaffolding for WebGPU Shading Language prototype : [Attachment 319284] the patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 30 00:07:09 PDT 2017


Keith Miller <keith_miller at apple.com> has denied  review:
Bug 175925: Initial reference implementation scaffolding for WebGPU Shading
Language prototype
https://bugs.webkit.org/show_bug.cgi?id=175925

Attachment 319284: the patch

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




--- Comment #30 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 319284
  --> https://bugs.webkit.org/attachment.cgi?id=319284
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.


More information about the webkit-reviews mailing list