Bug 187728 – Minor changes to the WHLSL interpreter to support the new compiler

• Component: New Bugs
• Status: RESOLVED FIXED

[Thomas Denney]

Minor changes to the WHLSL interpreter to support the new compiler

[Thomas Denney]

Created attachment 345161 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 345161 [details]
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/

[Thomas Denney]

Comment on attachment 345161 [details]
Patch

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

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

FunctionLikeBlocks are only constructed with functions that have been instantiated.

>> Tools/WebGPUShadingLanguageRI/NativeFunc.js:41
>> +    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.

implementationData is used in a few other places in the interpreter for this kind of thing (e.g. struct accessors) so I'm aiming to be consistent with them.

[Thomas Denney]

Created attachment 345472 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345472 [details]
Patch

Rejecting attachment 345472 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 345472, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/8642541

[Thomas Denney]

Created attachment 345719 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345719 [details]
Patch

Clearing flags on attachment: 345719

Committed r234175: <https://trac.webkit.org/changeset/234175>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42560713>