Bug 187728

Created attachment 345161 [details] Patch

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/

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.

Created attachment 345472 [details] Patch

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

Created attachment 345719 [details] Patch

Comment on attachment 345719 [details] Patch Clearing flags on attachment: 345719 Committed r234175: <https://trac.webkit.org/changeset/234175>

All reviewed patches have been landed. Closing bug.

<rdar://problem/42560713>