[Webkit-unassigned] [Bug 187728] Minor changes to the WHLSL interpreter to support the new compiler

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=187728

Myles C. Maxfield <mmaxfield at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #345161|review?                     |review+
              Flags|                            |

--- Comment #2 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 345161
  --> https://bugs.webkit.org/attachment.cgi?id=345161
Patch

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

Your SwizzleOp stuff could have been its own patch.

> Tools/WebGPUShadingLanguageRI/FuncInstantiator.js:34
> +    get instances() { return this._instances; }

Usually we don't make changes that are totally unused anywhere else in the patch. It's fine - just a note for the future.

> Tools/WebGPUShadingLanguageRI/FunctionLikeBlock.js:32
> +        this._func = func;

Is this right? I thought the function would not have its generic types be instantiated, but the FunctionLikeBlock would.

> Tools/WebGPUShadingLanguageRI/Intrinsics.js:665
> +                    for (let i = 0; i < swizzle.outSize; i++)
> +                        outputBuffer.set(i, vec.get(readIndices[swizzle.components[i]]));

Cool trick.

> Tools/WebGPUShadingLanguageRI/Lexer.js:106
> +                this._index += endIndex + 2;

Is there a test for this?

> Tools/WebGPUShadingLanguageRI/MakeArrayRefExpression.js:34
> +        if (!type) {
> +            if (this.lValue.variable && this.lValue.variable.type && this.lValue.variable.type.isArray && this.lValue.variable.type.elementType) {

You can join the if statements with &&.

This is very confusing because this argument is never used, so I have no idea whether or not this line is right. The ChangeLog helps, but in the future, please join all related changes in a single patch.

> Tools/WebGPUShadingLanguageRI/NativeFunc.js:41
> +    get implementationData() { return this._implementationData; }
> +    set implementationData(newImplData) { this._implementationData = newImplData; }

Isn't there a better name for this?
Also, I would sleep better at night if the constructor set it to undefined or null or something, just so it's easier to see all the members the class contains in a single place.

> Tools/WebGPUShadingLanguageRI/Rewriter.js:355
> +            Node.visit(node.func, this),
> +            node.argumentList.map(arg => Node.visit(arg, this))

It looks like the only difference between node.func.visit() and Node.visit(node.func) is that Node.visit() handles the case where the argument is null. When would that ever occur here? Shouldn't func always be non-null?

> Tools/WebGPUShadingLanguageRI/StandardLibrary.js:409
> +class SwizzleOp {

It's pretty weird for the standard library to include objects. I think you should move this out into another file (either another existing file if there is a good candidate, or a new file).

> Tools/WebGPUShadingLanguageRI/StandardLibrary.js:423
> +        return `native vec${this.outSize}<T> operator.${this.components.join("")}<T>(vec${this.inSize}<T> v)`;

Consider adding a performance test, because compiling the standard library is something we want to track.

> Tools/WebGPUShadingLanguageRI/StandardLibrary.js:427
> +// There are 481 swizzle operators. Let's not list them explicitly.

s/list them/compile them all/

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180720/cd099543/attachment-0001.html>