[Webkit-unassigned] [Bug 187735] Adding WHLSL compiler

[bugzilla-daemon at webkit.org]

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

Dean Jackson <dino at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dino at apple.com
 Attachment #345174|review?                     |review+
              Flags|                            |

--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 345174
  --> https://bugs.webkit.org/attachment.cgi?id=345174
Patch

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

Can we have some tests that check input v expected output?

> Tools/WebGPUShadingLanguageRI/Metal/ArrayRefDefinition.ts:26
> +/// Writes the definitions of the structures used for the different array ref types present in the program

While this isn't C++ code, and also isn't JS code, we should probably still follow the coding guidelines. That would mean using // and ending the sentence with a full stop (period). The same goes for other comments in the patch.

> Tools/WebGPUShadingLanguageRI/Metal/Deinliner.ts:48
> +        if (a == ReturnStyle.Always || a == ReturnStyle.SometimesEarly) {
> +            return a;
> +        } else {
> +            return b;
> +        }

Single line conditionals.

> Tools/WebGPUShadingLanguageRI/Metal/Deinliner.ts:138
> +            if (result == ReturnStyle.Always) {
> +                return ReturnStyle.SometimesEarly;
> +            }

Single line conditionals don't use {} in WebKit.

> Tools/WebGPUShadingLanguageRI/Metal/Deinliner.ts:186
> +    const returnStyle = returnStyleForNodeOrNull(node);
> +    return returnStyle != ReturnStyle.SometimesEarly;

No need for temporary local variable.

> Tools/WebGPUShadingLanguageRI/Metal/FindTopLevelTypeAttributes.ts:69
> +        case "vertex":
> +            this.visitVertexShader(func);
> +        case "fragment":
> +            this.visitFragmentShader(func);

I assume TypeScript doesn't need break in cases?

> Tools/WebGPUShadingLanguageRI/Metal/FindTopLevelTypeAttributes.ts:78
> +        for (let param of func.parameters) {
> +            this.attributesForType(param.type).isVertexAttribute = true;
> +        }

Single line statement.

> Tools/WebGPUShadingLanguageRI/Metal/FindTopLevelTypeAttributes.ts:85
> +        for (let param of func.parameters) {
> +            this.attributesForType(param.type).isVertexOutputOrFragmentInput = true;
> +        }

Ditto.

> Tools/WebGPUShadingLanguageRI/Metal/LineNumbers.ts:29
> +    const lineNos = [];
> +    let lineNo = 1;

Use lineNumbers and lineNumber.

> Tools/WebGPUShadingLanguageRI/Metal/LineNumbers.ts:34
> +        if (src[i] == '\n') {
> +            lineNo++;
> +        }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.ts:60
> +        for (let param of this._func.parameters) {
> +            map.set(param.name, `P${counter++}`);
> +        }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.ts:69
> +        for (let [paramName, param] of this.paramMap) {
> +            params.push(paramName);
> +        }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.ts:85
> +        if (this._func.shaderType === "vertex" || this._func.shaderType === "fragment") {
> +            declLine += `${this._func.shaderType} `;
> +        }

Should you return if it isn't one of these types?

> Tools/WebGPUShadingLanguageRI/Metal/MSLLexer.ts:87
> +                    for (let innerTok of this._lex(commentText, this.commentMatchers)) {
> +                        tokens.push(innerTok);
> +                    }

SIngle line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLLexer.ts:105
> +                    } else {
> +                        tokens.push(new Token(matcher.type, match[0]));
> +                    }

Ditto.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:74
> +        if (this.ancestorReturnType.name === "void") {
> +            this._add(`return;`);
> +        } else {
> +            const resultVar = this._fresh();
> +            this._add(`${this._typeNamer.mslTypeName(this.ancestorReturnType)} ${resultVar};`);
> +            this._zeroInitialize(this.ancestorReturnType, resultVar);
> +            this._add(`return ${resultVar};`);
> +        }

For things like this we normally do early returns. e.g.

if (this.ancestorReturnType.name === "void") {
  this._add(`return;`);
  return;
}

const resultVar....

Also, you don't need a `` string for the "return". I'm not sure there is really a performance win though.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:91
> +                if (node.name == "bool") {
> +                    emitter._add(`${variableName} = false;`);
> +                } else {
> +                    emitter._add(`${variableName} = 0;`);
> +                }
> +            }

Single line conditionals.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:102
> +                for (let i = 0; i < node.numElements.value; i++) {
> +                    emitter._zeroInitialize(node.elementType, `${variableName}[${i}]`, false);
> +                }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:244
> +        } else {
> +            this._add('}');
> +        }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:263
> +        } else {
> +            throw new Error(`Couldn't determine type of ! expression ${node}'`);
> +        }

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:277
> +        if (this._canAvoidDereferenceBecauseDereferencingReferenceNode(node.ptr)) {
> +            return this._doNotProduceReferenceFromReferenceNode(node.ptr);
> +        } else {

Single line.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:370
> +                if (anderRegex.test(node.nativeFuncInstance.name)) {
> +                    return emitter._handleFieldAccess(node);
> +                } else if (node.nativeFuncInstance.name === "operator&[]") {
> +                    return emitter._handleIndexAccess(node);
> +                }

Again.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:425
> +        } else {
> +            return this._nameMap.get(node.name);
> +        }

Again.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.ts:441
> +        for (let expr of node.list) {
> +            result = Node.visit(expr, this);
> +        }

Again. I'll stop pointing them out now.

-- 
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/20180717/c49d2b25/attachment.html>