[webkit-reviews] review granted: [Bug 187728] Minor changes to the WHLSL interpreter to support the new compiler : [Attachment 345161] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Thomas Denney
<tdenney at apple.com>'s request for review:
Bug 187728: Minor changes to the WHLSL interpreter to support the new compiler
https://bugs.webkit.org/show_bug.cgi?id=187728

Attachment 345161: Patch

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




--- 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/