Bug 187735 – [WHLSL] Metal code generation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Thomas Denney]

Adding WHLSL compiler

[Thomas Denney]

Created attachment 345174 [details]
Patch

[EWS Watchlist]

Attachment 345174 [details] did not pass style-queue:


ERROR: Tools/WebGPUShadingLanguageRI/Metal/tsconfig.json:14:  Expecting property name enclosed in double quotes: line 14 column 9 (char 309)  [json/syntax] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 345174 [details]
Patch

Attachment 345174 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8566221

New failing tests:
fast/loader/plain-text-document.html

[EWS Watchlist]

Created attachment 345185 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dean Jackson]

Comment on attachment 345174 [details]
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.

[Thomas Denney]

Created attachment 345208 [details]
Patch

[EWS Watchlist]

Attachment 345208 [details] did not pass style-queue:


ERROR: Tools/WebGPUShadingLanguageRI/Metal/tsconfig.json:14:  Expecting property name enclosed in double quotes: line 14 column 9 (char 309)  [json/syntax] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thomas Denney]

Created attachment 345219 [details]
Patch

[Thomas Denney]

Created attachment 345220 [details]
Patch

[Thomas Denney]

Created attachment 348257 [details]
WIP

[Thomas Denney]

Created attachment 348407 [details]
WIP

[Thomas Denney]

Created attachment 348703 [details]
Patch

[Thomas Denney]

Created attachment 348852 [details]
Patch sans TypeScript

[Thomas Denney]

Created attachment 348869 [details]
Patch sans TypeScript

[EWS Watchlist]

Comment on attachment 348869 [details]
Patch sans TypeScript

Attachment 348869 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9096384

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 348884 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Myles C. Maxfield]

Comment on attachment 348869 [details]
Patch sans TypeScript

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

Halfway done reviewing

> Tools/WebGPUShadingLanguageRI/MakeArrayRefExpression.js:40
> +            while (lValueType instanceof TypeRef && lValueType.type)
> +                lValueType = lValueType.type;

Why is this necessary to do in the constructor? Our code should be resilient to TypeRefs.

> Tools/WebGPUShadingLanguageRI/MakeArrayRefExpression.js:44
> +            if (lValueType.isArray && lValueType.elementType)
> +                this._type = new ArrayRefType(origin, "thread", lValueType.elementType);
> +            else
> +                this._type = new ArrayRefType(origin, "thread", lValueType);

Can't we do this with a virtual function instead of hardcoding type names?

> Tools/WebGPUShadingLanguageRI/Metal/ArrayRefDefinition.js:39
> +function metalSourceForArrayRefDefinition(arrayRefType, typeNamer)
> +{
> +    let src = `struct ${typeNamer.uniqueTypeId(arrayRefType)} {\n`;
> +    const fakePtrType = new PtrType(arrayRefType.origin, arrayRefType.addressSpace, arrayRefType.elementType);
> +    src += `    ${metalSourceForVarDeclaration(typeNamer, fakePtrType, "ptr")};\n`;
> +    src += "    uint32_t length;\n";
> +    src += "};";
> +    return src;
> +}
> +
> +function metalSourceForArrayRefForwardDeclaration(arrayRefType, typeNamer)
> +{
> +    return `struct ${typeNamer.uniqueTypeId(arrayRefType)};`;
> +}

Why is this in its own file? It's only called in one place.

> Tools/WebGPUShadingLanguageRI/Metal/CompileResult.js:26
> +class CompileResult {

Does Javascript have the concept of namespaces? We want to make sure that the Metal backend's classes don't collide with any other codegen's classes. If there aren't real namespaces, we should prefix the names that get put in the global scope (probably except for whlslToMsl()).

> Tools/WebGPUShadingLanguageRI/Metal/ConstexprEmitter.js:37
> +    visitEnumLiteral(node)
> +    {
> +        return node.member.value.toString();
> +    }

Does this actually work? Is this tested?

We should make sure this file and https://bugs.webkit.org/show_bug.cgi?id=189029 stay in sync

> Tools/WebGPUShadingLanguageRI/Metal/FunctionLikeBlockCanBeInlined.js:28
> +function functionLikeBlockCanBeInlined(node)

We should make a bug to add whatever infrastructure is necessary to allow inlining for every function.

> Tools/WebGPUShadingLanguageRI/Metal/LineNumbers.js:38
> +function lineNumbers(src)
> +{
> +    const lineNumbers = [];
> +    let lineNumber = 1;
> +    for (let i = 0; i < src.length; i++) {
> +        lineNumbers.push(lineNumber);
> +        if (src[i] == '\n')
> +            lineNumber++;
> +    }
> +    return lineNumbers;
> +}

Why not use the .origin property on parsed nodes? For every expression, you'll know where it came from in the source. No need to calculate it twice.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:31
> +        this._funcMangler = funcMangler;

This is a layering inversion. The mangler is only used here for a single place; why doesn't whoever constructs a MSLFunctionDeclaration pass in the mangled name? This could also apply to the other arguments here.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:41
> +    get funcMangler()
> +    {
> +        return this._funcMangler;
> +    }

Similar to above; this seems upside-down.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:94
> +        // What WSL calls `compute` shaders Metal calls `kernel` shaders.
> +        // We're only focussing on `vertex` and `fragment` for now.

Do we have a bug for this? Seems important.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:99
> +            declLine += `${this._func.shaderType} `;
> +        declLine += `${this._typeNamer.mslTypeName(this._func.returnType)} `;
> +        declLine += this._funcMangler.mangle(this.func);
> +        declLine += "("

Doesn't Javascript have a better way to do string building? Even appending a bunch of strings to an array and then joining the array at the last minute would be better.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:120
> +        // We currently assuming that all parameters to entry points are attributes.

:(

Open a bug?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:40
> +        this._counter = counter ? counter : new Counter("V");

Seems over-engineered. Getters/setters of a Number seems simpler.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:714
> +        if (canBeInlined)

If it can be inlined, then inline it? That seems like a non-optimal heuristic. We should have an explicit step to record whether or not each function should be inlined, and then this step will just consult that list.

Initially, inlining nothing seems like the safest bet.

> Tools/WebGPUShadingLanguageRI/Metal/MSLTypeNamer.js:66
> +                    "void": "void",
> +                    "bool": "bool",
> +                    "uchar": "uint8_t",
> +                    "ushort": "uint16_t",
> +                    "uint": "uint32_t",
> +                    "char": "int8_t",
> +                    "short": "int16_t",
> +                    "int": "int32_t",
> +                    "half": "half",
> +                    "float": "float",
> +                    "atomic_int": "atomic_int",
> +                    "atomic_uint": "atomic_uint"

Samplers, textures

> Tools/WebGPUShadingLanguageRI/Metal/MSLTypeNamer.js:129
> +                    "bool": "bool",

Bool matrices don't exist

> Tools/WebGPUShadingLanguageRI/Metal/MetalCodegen.js:84
> +    _tryGenerateMsl()

Functions that don't return a sentinel don't need to be named "try"

> Tools/WebGPUShadingLanguageRI/Metal/MetalCodegen.js:86
> +        // Step 1: Find all the functions to compile

Comments should answer "why," not "how." I recommend deleting these.

> Tools/WebGPUShadingLanguageRI/Metal/MetalCodegen.js:119
> +    _findEntryPointDefinitions()

How about findReachableFunctions()?

> Tools/WebGPUShadingLanguageRI/Metal/MetalCodegen.js:188
> +            } else if (type instanceof ArrayRefType) {
> +                this._forwardTypeDecls.push(metalSourceForArrayRefForwardDeclaration(type, this._typeNamer));
> +                this._typeDefinitions.push(metalSourceForArrayRefDefinition(type, this._typeNamer));

We should probably allow Arrays (in addition to ArrayRefs). They are strictly less powerful.

> Tools/WebGPUShadingLanguageRI/Metal/Token.js:26
> +// The output of MSLLexer.

This notion is better encapsulated with names (either of variables or the class)

> Tools/WebGPUShadingLanguageRI/Metal/TypeOf.js:26
> +function typeOf(node) {

I'm not sure why this is necessary. I think we should make the Checker always attach the type of everything to itself. It could be implemented with a getter that's overridden by every class.

> Tools/WebGPUShadingLanguageRI/Metal/TypeOf.js:131
> +            return node.type.visit(this);

Doesn't seem right. The type of an array ref is not equal to the inner type.

> Tools/WebGPUShadingLanguageRI/Metal/TypeOf.js:226
> +            throw new Error("BoolLiteral has no type");

Why isn't the type of a BoolLiteral a bool? NullLiteral has a type...

> Tools/WebGPUShadingLanguageRI/Metal/TypeUnifier.js:75
> +        node.baseType.visit(this);

Why not call super? (And ditto for all the other occurrences in this file)

> Tools/WebGPUShadingLanguageRI/Metal/TypeUnifier.js:96
> +        return `${node.elementType.visit(this)}[${node.numElements}]`;

I think this has been renamed to numElementsValue

> Tools/WebGPUShadingLanguageRI/Metal/TypeUnifier.js:118
> +    visitMakeArrayRefExpression(node)
> +    {
> +        if (!node.type)
> +            throw new Error(node);
> +        return node.type.visit(this);
> +    }

Why just MakeArrayRefExpression? Shouldn't we be visiting every expression? Similarly, we should make sure every expression has a .type property.

> Tools/WebGPUShadingLanguageRI/Metal/TypeUnifier.js:123
> +        const nameSet = new Set();

Seems like this set will always have the exact same set of things as in declSet. Why have two?

> Tools/WebGPUShadingLanguageRI/Metal/WHLSLLexer.js:71
> +function lexWHLSL(src)

We already have a WHLSL lexer. Why reimplement it?

> Tools/WebGPUShadingLanguageRI/Metal/compiler.js:11
> +/*
> + * Copyright (C) 2018 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.

I'd recommend a better name for this file. Also, using correct casing.

> Tools/WebGPUShadingLanguageRI/Metal/index.html:173
> +    <script src="../Node.js"></script>
> +    <script src="../Type.js"></script>
> +    <script src="../ReferenceType.js"></script>
> +    <script src="../Value.js"></script>
> +    <script src="../Expression.js"></script>
> +    <script src="../Rewriter.js"></script>
> +    <script src="../Visitor.js"></script>
> +    <script src="../CreateLiteral.js"></script>
> +    <script src="../CreateLiteralType.js"></script>
> +    <script src="../PropertyAccessExpression.js"></script>
> +    <script src="../NativeType.js"></script>
> +
> +    <script src="../AddressSpace.js"></script>
> +    <script src="../AnonymousVariable.js"></script>
> +    <script src="../ArrayRefType.js"></script>
> +    <script src="../ArrayType.js"></script>
> +    <script src="../Assignment.js"></script>
> +    <script src="../AutoWrapper.js"></script>
> +    <script src="../Block.js"></script>
> +    <script src="../BoolLiteral.js"></script>
> +    <script src="../Break.js"></script>
> +    <script src="../BuiltinMatrixGetter.js"></script>
> +    <script src="../BuiltinMatrixSetter.js"></script>
> +    <script src="../BuiltinVectorGetter.js"></script>
> +    <script src="../BuiltinVectorSetter.js"></script>
> +    <script src="../CallExpression.js"></script>
> +    <script src="../CallFunction.js"></script>
> +    <script src="../Check.js"></script>
> +    <script src="../CheckLiteralTypes.js"></script>
> +    <script src="../CheckLoops.js"></script>
> +    <script src="../CheckRecursion.js"></script>
> +    <script src="../CheckRecursiveTypes.js"></script>
> +    <script src="../CheckReturns.js"></script>
> +    <script src="../CheckTypesWithArguments.js"></script>
> +    <script src="../CheckUnreachableCode.js"></script>
> +    <script src="../CheckWrapped.js"></script>
> +    <script src="../Checker.js"></script>
> +    <script src="../CloneProgram.js"></script>
> +    <script src="../CommaExpression.js"></script>
> +    <script src="../ConstexprFolder.js"></script>
> +    <script src="../Continue.js"></script>
> +    <script src="../ConvertPtrToArrayRefExpression.js"></script>
> +    <script src="../DoWhileLoop.js"></script>
> +    <script src="../DotExpression.js"></script>
> +    <script src="../DereferenceExpression.js"></script>
> +    <script src="../EArrayRef.js"></script>
> +    <script src="../EBuffer.js"></script>
> +    <script src="../EBufferBuilder.js"></script>
> +    <script src="../EPtr.js"></script>
> +    <script src="../EnumLiteral.js"></script>
> +    <script src="../EnumMember.js"></script>
> +    <script src="../EnumType.js"></script>
> +    <script src="../EvaluationCommon.js"></script>
> +    <script src="../Evaluator.js"></script>
> +    <script src="../ExpressionFinder.js"></script>
> +    <script src="../ExternalOrigin.js"></script>
> +    <script src="../Field.js"></script>
> +    <script src="../FindHighZombies.js"></script>
> +    <script src="../FlattenedStructOffsetGatherer.js"></script>
> +    <script src="../FloatLiteral.js"></script>
> +    <script src="../FloatLiteralType.js"></script>
> +    <script src="../FoldConstexprs.js"></script>
> +    <script src="../ForLoop.js"></script>
> +    <script src="../Func.js"></script>
> +    <script src="../FuncDef.js"></script>
> +    <script src="../FuncParameter.js"></script>
> +    <script src="../FunctionLikeBlock.js"></script>
> +    <script src="../HighZombieFinder.js"></script>
> +    <script src="../IdentityExpression.js"></script>
> +    <script src="../IfStatement.js"></script>
> +    <script src="../IndexExpression.js"></script>
> +    <script src="../InferTypesForCall.js"></script>
> +    <script src="../Inline.js"></script>
> +    <script src="../Inliner.js"></script>
> +    <script src="../IntLiteral.js"></script>
> +    <script src="../IntLiteralType.js"></script>
> +    <script src="../Intrinsics.js"></script>
> +    <script src="../LateChecker.js"></script>
> +    <script src="../Lexer.js"></script>
> +    <script src="../LexerToken.js"></script>
> +    <script src="../LiteralTypeChecker.js"></script>
> +    <script src="../LogicalExpression.js"></script>
> +    <script src="../LogicalNot.js"></script>
> +    <script src="../LoopChecker.js"></script>
> +    <script src="../MakeArrayRefExpression.js"></script>
> +    <script src="../MakePtrExpression.js"></script>
> +    <script src="../MatrixType.js"></script>
> +    <script src="../NameContext.js"></script>
> +    <script src="../NameFinder.js"></script>
> +    <script src="../NameResolver.js"></script>
> +    <script src="../NativeFunc.js"></script>
> +    <script src="../NormalUsePropertyResolver.js"></script>
> +    <script src="../NullLiteral.js"></script>
> +    <script src="../NullType.js"></script>
> +    <script src="../OperatorAnderIndexer.js"></script>
> +    <script src="../OperatorArrayRefLength.js"></script>
> +    <script src="../OriginKind.js"></script>
> +    <script src="../OverloadResolutionFailure.js"></script>
> +    <script src="../Parse.js"></script>
> +    <script src="../Prepare.js"></script>
> +    <script src="../PropertyResolver.js"></script>
> +    <script src="../Program.js"></script>
> +    <script src="../ProgramWithUnnecessaryThingsRemoved.js"></script>
> +    <script src="../PtrType.js"></script>
> +    <script src="../ReadModifyWriteExpression.js"></script>
> +    <script src="../RecursionChecker.js"></script>
> +    <script src="../RecursiveTypeChecker.js"></script>
> +    <script src="../ResolveNames.js"></script>
> +    <script src="../ResolveOverloadImpl.js"></script>
> +    <script src="../ResolveProperties.js"></script>
> +    <script src="../ResolveTypeDefs.js"></script>
> +    <script src="../Return.js"></script>
> +    <script src="../ReturnChecker.js"></script>
> +    <script src="../ReturnException.js"></script>
> +    <script src="../StandardLibrary.js"></script>
> +    <script src="../StatementCloner.js"></script>
> +    <script src="../StructLayoutBuilder.js"></script>
> +    <script src="../StructType.js"></script>
> +    <script src="../SwitchCase.js"></script>
> +    <script src="../SwitchStatement.js"></script>
> +    <script src="../SynthesizeArrayOperatorLength.js"></script>
> +    <script src="../SynthesizeEnumFunctions.js"></script>
> +    <script src="../SynthesizeStructAccessors.js"></script>
> +    <script src="../SynthesizeCopyConstructorOperator.js"></script>
> +    <script src="../SynthesizeDefaultConstructorOperator.js"></script>
> +    <script src="../TernaryExpression.js"></script>
> +    <script src="../TrapStatement.js"></script>
> +    <script src="../TypeDef.js"></script>
> +    <script src="../TypeDefResolver.js"></script>
> +    <script src="../TypeRef.js"></script>
> +    <script src="../TypeOverloadResolutionFailure.js"></script>
> +    <script src="../TypedValue.js"></script>
> +    <script src="../UintLiteral.js"></script>
> +    <script src="../UintLiteralType.js"></script>
> +    <script src="../UnificationContext.js"></script>
> +    <script src="../UnreachableCodeChecker.js"></script>
> +    <script src="../VariableDecl.js"></script>
> +    <script src="../VariableRef.js"></script>
> +    <script src="../VectorType.js"></script>
> +    <script src="../VisitingSet.js"></script>
> +    <script src="../WSyntaxError.js"></script>
> +    <script src="../WTrapError.js"></script>
> +    <script src="../WTypeError.js"></script>
> +    <script src="../WhileLoop.js"></script>
> +    <script src="../WrapChecker.js"></script>
> +
> +    <script src="ArrayRefDefinition.js"></script>
> +    <script src="CompileResult.js"></script>
> +    <script src="ConstexprEmitter.js"></script>
> +    <script src="Counter.js"></script>
> +    <script src="FunctionLikeBlockCanBeInlined.js"></script>
> +    <script src="LineNumbers.js"></script>
> +    <script src="MSLFunctionDeclaration.js"></script>
> +    <script src="MSLFunctionDefinition.js"></script>
> +    <script src="MSLFunctionForwardDeclaration.js"></script>
> +    <script src="MSLLexer.js"></script>
> +    <script src="MSLStatementEmitter.js"></script>
> +    <script src="MSLTypeNamer.js"></script>
> +    <script src="MetalCodegen.js"></script>
> +    <script src="NameMangler.js"></script>
> +    <script src="SortTypeDeclarations.js"></script>
> +    <script src="StructDefinition.js"></script>
> +    <script src="Token.js"></script>
> +    <script src="TypeAttributeMap.js"></script>
> +    <script src="TypeAttributes.js"></script>
> +    <script src="TypeOf.js"></script>
> +    <script src="TypeUnifier.js"></script>
> +    <script src="VarDeclaration.js"></script>
> +    <script src="WHLSLLexer.js"></script>

We've really got to find a better way to do this. https://bugs.webkit.org/show_bug.cgi?id=189378

> Tools/WebGPUShadingLanguageRI/StandardLibrary.js:43
> +        for (var type of [`bool`, `uchar`, `ushort`, `uint`, `char`, `short`, `int`, `half`, `float`]) {
> +            for (var size of [2, 3, 4]) {

:(

> Tools/WebGPUShadingLanguageRI/Test.js:35
> +} else if (this.load)

Why?

> Tools/WebGPUShadingLanguageRI/Test.js:74
>  {
> -    return TypedValue.box(program.intrinsics.int, value);
> +    return TypedValue.box(program.intrinsics.int, (value | 0));
>  }
>  
>  function makeUint(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.uint, value);
> +    return TypedValue.box(program.intrinsics.uint, (value >>> 0));
>  }
>  
>  function makeUchar(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.uchar, value);
> +    return TypedValue.box(program.intrinsics.uchar, (value >>> 0) & 0xff);
>  }
>  
>  function makeBool(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.bool, value);
> +    return TypedValue.box(program.intrinsics.bool, !!value);
>  }

These have been moved to Casts.js

> Tools/WebGPUShadingLanguageRI/Test.js:1080
> -    checkNumber(program, callFunction(program, "foo", []), 42);
> +    checkUint(program, callFunction(program, "foo", [], []), 42);

Are you are the extra argument to callFunction is right?

Can we delete checkNumber()?

> Tools/WebGPUShadingLanguageRI/Test.js:1086
> -    checkNumber(program, callFunction(program, "foo", []), 42);
> +    checkFloat(program, callFunction(program, "foo", [], []), 42);

Ditto.

> Tools/WebGPUShadingLanguageRI/Test.js:-2421
> -
> -    checkFail(
> -        () => doPrep(`
> -            void foo()
> -            {
> -                float3 x;
> -                bool r = bool(x);
> -            }
> -        `),
> -        e => e instanceof WTypeError);
> -
> -    checkFail(
> -        () => doPrep(`
> -            void foo()
> -            {
> -                float3x3 x;
> -                bool r = bool(x);
> -            }
> -        `),
> -        e => e instanceof WTypeError);

I've since deleted this, I think you'll need to rebase.

> Tools/WebGPUShadingLanguageRI/Test.js:-2711
> -    checkUchar(program, callFunction(program, "foo", [makeUchar(program, 65535), makeUint(program, 2)]), 252);

Why?

> Tools/WebGPUShadingLanguageRI/Test.js:-2726
> -    checkUchar(program, callFunction(program, "foo", [makeUchar(program, 65535), makeUint(program, 2)]), 255);
> -    checkUchar(program, callFunction(program, "foo", [makeUchar(program, -1), makeUint(program, 5)]), 255);

Why?

[Myles C. Maxfield]

Comment on attachment 348869 [details]
Patch sans TypeScript

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

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:210
> +        // This is necessary because for loops are compiled as while loops, so we need to do
> +        // the loop increment and condition check before returning to the beginning of the loop.

Why? For loops are implemented in the interpreter, why shouldn't they be implemented in Metal?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:211
> +        this._emitLoopBodyEnd();

Wha happens if a continue is inside a switch inside a loop?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:221
> +        for (let stmt of block.statements) {

No need to smoosh together "statement" into "stmt"

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:225
> +        }

Why don't blocks emit "{"?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:232
> +        if (this._resultVar)

What's the difference between _resultVar and the returned value?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:235
> +            this._add(`return ${expr};`);

All expressions should be saved into variables.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:291
> +        const type = typeOf(node);
> +        if (type) {

Should just assume this is bool. We prove that elsewhere.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:306
> +        if (this._canAvoidDereferenceBecauseDereferencingReferenceNode(node.ptr))
> +            return this._doNotProduceReferenceFromReferenceNode(node.ptr);

I'd recommend putting this kind of thing in a follow-up patch.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:308
> +            return `(*(${node.ptr.visit(this)}))`;

All expressions should be saved into local variables.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:427
> +        if (node.variable instanceof AnonymousVariable)
> +            return Node.visit(node.variable, this);
> +        let name = node.name;
> +        if (!this._nameMap.has(name))
> +            throw new Error(`${node.name} not found in this function's variable map`);

We should be mapping variables to names, not names to names. That way we can unify these paths.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:467
> +        // Anonymous variable names are of the form anonVar<i> (where i is an integer), i.e.
> +        // the set of anonymous variable names is disjoint from the set of definable variable names.
> +        if (!this._nameMap.has(node.name)) {
> +            let id = this._fresh();
> +            this._nameMap.set(node.name, id);
> +            this._add(metalSourceForVarDeclaration(this._typeNamer, node.type, id) + `; // ${node.name}`);
> +            this._zeroInitialize(node.type, id);
> +            return id;
> +        } else
> +            return this._nameMap.get(node.name);

Anonymous variables should be treated exactly the same as regular variables.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:475
> +        return rhs;

I think you mean lhs here

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:540
> +        else
> +            throw new Error(`Unimplemented native function '${node.name}' at MSLStatementEmitter.makeFunctionCall`);

Without inlining, this is no longer correct.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:565
> +        this._emitReturnZeroValue();

This isn't right. This should be a trap, the whole program should stop executing. We want more than just the current function to return zeroes.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:575
> +        this._emitReturnZeroValue();

Ditto.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:606
> +        this._emitReturnZeroValue();

Ditto

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:624
> +        if (arg instanceof MakePtrExpression) {
> +            const argId = Node.visit(arg.lValue, this);
> +            if (arg.lValue && arg.lValue instanceof VariableRef)
> +                return `${argId}.${field}`;
> +            else
> +                return `(${argId}).${field}`;
> +        } else
> +            return `${arg.visit(this)}->${field}`;

Why does the field access care what type the inner thing is? We should try to make it so that we can just do a recursive call here and the right thing happens.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:632
> +    _emitOperatorAnder(node, result, args)
> +    {
> +        const type = node.implementationData.type;
> +        const field = this._typeAttributes.attributesForType(type).mangledFieldName(node.implementationData.name);
> +        this._add(`${result} = (${this._typeNamer.mslTypeName(node.returnType)})&((${args[0]})->${field});`);
> +    }

Which anders aren't implemented in the language?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:646
> +        // FIXME: Avoid so much special casing.

Yes please. See the above comment for _handleFieldAccess

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:685
> +        const expr = node.lValue.visit(this);
> +        return `&(${expr})`;

Why doesn't this get saved into a variable?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:732
> +        const emitter = new MSLStatementEmitter(this._funcMangler, this._typeNamer, node.body, nameMap, node.origin.text, this._lineNumbers, this._typeAttributes, resultVar, this._counter, this.ancestorReturnType);

Why is it necessary to create a new MSLStatementEmitter? Why can't we just keep recursing?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:743
> +    // which is checked on each iteration (all loops are compiled to a while loop or do/while loop). The check is

Why?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:749
> +            Node.visit(node.initialization, this); // initalization can be null

No need for this comment

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:866
> +        return `(nullptr)`;

Why parentheses?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:928
> +        this._indent();

There should be a function which takes a lambda that does the indent/unindent for you

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:944
> +    // Unimplemented methods. I write out comments for these rather than throwing exceptions because it makes it easier for me
> +    // to see where the nodes occur in the abstract syntax tree

We should throw exceptions for these. Writing out the location might be valuable for your own debugging, but it isn't for committing

[Thomas Denney]

(In reply to Myles C. Maxfield from comment #18)
> Comment on attachment 348869 [details]
> Patch sans TypeScript
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348869&action=review
> 
> > Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:210
> > +        // This is necessary because for loops are compiled as while loops, so we need to do
> > +        // the loop increment and condition check before returning to the beginning of the loop.
> 
> Why? For loops are implemented in the interpreter, why shouldn't they be
> implemented in Metal?

The initializer, condition, or the increment of the for loop may be a compound expression in WHLSL that has to compile to several statements in Metal. It is awkward to retain the for loop structure in Metal.

[Myles C. Maxfield]

(In reply to Thomas Denney from comment #19)
> (In reply to Myles C. Maxfield from comment #18)
> > Comment on attachment 348869 [details]
> > Patch sans TypeScript
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=348869&action=review
> > 
> > > Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:210
> > > +        // This is necessary because for loops are compiled as while loops, so we need to do
> > > +        // the loop increment and condition check before returning to the beginning of the loop.
> > 
> > Why? For loops are implemented in the interpreter, why shouldn't they be
> > implemented in Metal?
> 
> The initializer, condition, or the increment of the for loop may be a
> compound expression in WHLSL that has to compile to several statements in
> Metal. It is awkward to retain the for loop structure in Metal.

👍

[Thomas Denney]

(In reply to Myles C. Maxfield from comment #17)
> Comment on attachment 348869 [details]
> Patch sans TypeScript
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348869&action=review

> > Tools/WebGPUShadingLanguageRI/Test.js:-2421
> > -
> > -    checkFail(
> > -        () => doPrep(`
> > -            void foo()
> > -            {
> > -                float3 x;
> > -                bool r = bool(x);
> > -            }
> > -        `),
> > -        e => e instanceof WTypeError);
> > -
> > -    checkFail(
> > -        () => doPrep(`
> > -            void foo()
> > -            {
> > -                float3x3 x;
> > -                bool r = bool(x);
> > -            }
> > -        `),
> > -        e => e instanceof WTypeError);
> 
> I've since deleted this, I think you'll need to rebase.

They’re still in master.

[Thomas Denney]

Created attachment 349505 [details]
WIP

[EWS Watchlist]

Comment on attachment 349505 [details]
WIP

Attachment 349505 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9187455

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 349524 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Thomas Denney]

(In reply to Myles C. Maxfield from comment #18)
> Comment on attachment 348869 [details]
> Patch sans TypeScript
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348869&action=review

> > Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:211
> > +        this._emitLoopBodyEnd();
> 
> Wha happens if a continue is inside a switch inside a loop?

I think this is OK. In WHLSL/C/C++ using a continue in a switch in a loop still affects the outer loop. _emitLoopBodyEnd just emits the loop increment statement and the condition statement, so the semantics are retained.

[Radar WebKit Bug Importer]

<rdar://problem/44403031>

[Thomas Denney]

Created attachment 349612 [details]
WIP

[Thomas Denney]

Created attachment 349696 [details]
WIP

[Thomas Denney]

(In reply to Myles C. Maxfield from comment #17)
> Comment on attachment 348869 [details]
> Patch sans TypeScript
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348869&action=review

> > Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:31
> > +        this._funcMangler = funcMangler;
> 
> This is a layering inversion. The mangler is only used here for a single
> place; why doesn't whoever constructs a MSLFunctionDeclaration pass in the
> mangled name? This could also apply to the other arguments here.
> 
> > Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:41
> > +    get funcMangler()
> > +    {
> > +        return this._funcMangler;
> > +    }
> 
> Similar to above; this seems upside-down.

MSLFunctionForwardDeclaration and MSLFunctionDefinition  are concrete subclasses of this class, and they each use all of the parameters; the former uses them for correctly constructing all of the parameters (e.g. with attributes) whilst the latter needs to pass all of them to its internal MSLStatementEmitter, which then needs to use the function name mangler for function calls, the parameter map for extending to the internal variable map, and the type attributes for correctly accessing type properties.

[Thomas Denney]

Created attachment 349733 [details]
Patch

[Thomas Denney]

Created attachment 349735 [details]
Patch

[Thomas Denney]

Created attachment 349817 [details]
Patch

[Thomas Denney]

Created attachment 349826 [details]
Patch

[EWS Watchlist]

Comment on attachment 349826 [details]
Patch

Attachment 349826 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9222200

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 349839 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Thomas Denney]

Created attachment 349939 [details]
Patch

[Thomas Denney]

Created attachment 349957 [details]
Patch

[Thomas Denney]

Created attachment 350095 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 349957 [details]
Patch

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

What have we done to make sure "return" and "continue" and "break" all play nicely together, e.g. in a switch statement in a loop in an inlined function?

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:26
> +// Emits code for for the first line of a function declaration or definition.

for for

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:81
> +        let declLine = this.commentLine();

Seems weird to have a function declaration include a comment line. I suggest not calling commentLine() here, but instead calling it in all the places that call this function.

Also, having a global switch to control these comments would be useful, since in production we don't want any text in the original source to appear in the generated code.

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:97
> +            else if (this.parameterIsAttribute(param))
> +                pStr += " [[stage_in]]";

I thought only one thing could have [[stage_in]]? Should there be a stage where we gather and partition the inputs/outputs of the shader?

> Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:115
> +        // We currently assuming that all parameters to entry points are attributes.
> +        // TODO: Better logic for this, i.e. support samplers.

We have to do this before we present. Probably should file a bug.

> Tools/WebGPUShadingLanguageRI/Metal/MSLNativeFunctionCall.js:75
> +        "native uint f32tof16(float)" : () => `${resultVariable} = uint(static_cast<ushort>(half(${args[0]})));`,

Pretty sure this shouldn't be a static_cast. It places the result "stored in the low-half of the uint."

https://docs.microsoft.com/en-us/windows/desktop/direct3dhlsl/f32tof16

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:85
> +                emitter._zeroInitialize(node.baseType, variableName, false);

How does this work?
enum Foo : int {
    Bar = 1,
    Baz = 2
}
Can't really zero-initialize that type, can we :(

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:222
> +        // At the moment the parser emits Block nodes for the if and else bodies, so this allows us

Really?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:362
> +        this._nameMap.set(node, id);

Excellent

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:442
> +        if (this._isOperatorAnder(node))
> +            this._emitOperatorAnder(node, resultVariable, args);
> +        else if (node.implementationData instanceof BuiltinVectorGetter)
> +            this._emitBuiltinVectorGetter(node, resultVariable, args);
> +        else if (node.implementationData instanceof BuiltinVectorSetter)
> +            this._emitBuiltinVectorSetter(node, resultVariable, args);
> +        else if (node.implementationData instanceof BuiltinMatrixGetter)
> +            this._emitBuiltinMatrixGetter(node, resultVariable, args);
> +        else if (node.implementationData instanceof BuiltinMatrixSetter)
> +            this._emitBuiltinMatrixSetter(node, resultVariable, args);
> +        else if (this._isOperatorValue(node))
> +            this._add(`${resultVariable} = ${args[0]};`);
> +        else if (this._isOperatorLength(node))
> +            this._emitOperatorLength(node, resultVariable, args);
> +        else if (this._isOperatorSetter(node))
> +            this._emitOperatorSetter(node, resultVariable, args);
> +        else if (this._isOperatorGetter(node))
> +            this._emitOperatorGetter(node, resultVariable, args);
> +        else if (this._isOperatorIndexer(node))
> +            this._emitOperatorIndexer(node, resultVariable, args);
> +        else if (this._isOperatorCast(node))
> +            this._emitOperatorCast(node, resultVariable, args);
> +        else if (this._isUnaryOperator(node))
> +            this._add(`${resultVariable} = ${this._extractOperatorName(node)}(${args[0]});`);
> +        else if (this._isBinaryOperator(node))
> +            this._add(`${resultVariable} = ${args[0]} ${this._extractOperatorName(node)} ${args[1]};`);
> +        else
> +            this._add(mslNativeFunctionCall(node, resultVariable, args));

I like this.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:614
> +    // Switch statements.

No need for this. Please delete.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:641
> +    // Literals

Ditto.

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:714
> +            if (node.isLValue)
> +                this._add(`${resultVar} = &(${bodyExpression});`);

I thought we were getting rid of this?

> Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:722
> +            if (node.isLValue)
> +                this._add(`${resultVar} = &(${elseExpression});`);

Ditto

> Tools/WebGPUShadingLanguageRI/Metal/MSLTypeNamer.js:86
> +                name = `${node.addressSpace} ${typeNamer.mslTypeName(node.elementType)}*`;

Wouldn't it be easier to emit a collection of typedefs at the top of the MSL source for every used type in the entire program? You could do a prepass to populate it, so you wouldn't have to build up these long strings. It would work like how each WHLSL expression gets mapped to a MSL statement with exactly one operation - each used WHLSL type in the entire program would map to a MSL typedef with exactly one construction.

> Tools/WebGPUShadingLanguageRI/Metal/MSLTypeNamer.js:91
> +                name = `${node.addressSpace ? node.addressSpace + " " : ""}${typeNamer.mslTypeName(node.elementType)}*`;

Better to emit "thread" to be explicit.

> Tools/WebGPUShadingLanguageRI/Metal/MSLVarDeclaration.js:45
> +    type = typeOf(type); // Unwrap TypeRef if necessary
> +    if (type.isArray) {
> +        let arrayType = type;
> +        const sizes = [ arrayType.numElementsValue ];
> +        let elementType = typeOf(arrayType.elementType);
> +        while (elementType.isArray) {
> +            arrayType = elementType;
> +            sizes.push(arrayType.numElementsValue);
> +            elementType = arrayType.elementType;
> +        }
> +        if (treatArrayAsPointer)
> +            return `${arrayType.addressSpace ? arrayType.addressSpace : "thread"} ${typeNamer.mslTypeName(elementType)} (*${name})${sizes.slice(1).map(size => `[${size}]`).join('')}`;
> +        else
> +            return `${typeNamer.mslTypeName(elementType)} ${name}${sizes.map(size => `[${size}]`).join('')}`;
> +    } else if (type.isPtr)
> +        return (type).addressSpace + " " + mslVarDeclaration(typeNamer, (type).elementType, `(*${name})`);
> +    else
> +        return `${typeNamer.mslTypeName(type)} ${name}`;

If we typedef every used type, this logic could probably be much simpler.

> Tools/WebGPUShadingLanguageRI/Metal/TypeOf.js:27
> +// FIXME: Rather than being a separate function this should instead be a separate preparation phase that annotates
> +// each Node instance with a "type" property. https://bugs.webkit.org/show_bug.cgi?id=189611

Doesn't need to be a separate phase. Checker can do it.

> Tools/WebGPUShadingLanguageRI/Metal/TypeOf.js:47
> +            return node.type.visit(this);

I don't understand why there are recursive calls on the .type properties. Why not just return node.type?

> Tools/WebGPUShadingLanguageRI/PropertyResolver.js:172
> +        if (node.lValue.unifyNode instanceof DereferenceExpression)
> +            node.become(new IdentityExpression(node.lValue.unifyNode.ptr));

Can you remind me again why this is necessary? This could potentially have side effects around lvalue validation, and we want to make sure our compiler matches the spec exactly. I don't think this kind of optimization should be made without making sure it's not observable.

[Thomas Denney]

(In reply to Myles C. Maxfield from comment #39)
> Comment on attachment 349957 [details]
> > Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:97
> > +            else if (this.parameterIsAttribute(param))
> > +                pStr += " [[stage_in]]";
> 
> I thought only one thing could have [[stage_in]]? Should there be a stage
> where we gather and partition the inputs/outputs of the shader?
> 
> > Tools/WebGPUShadingLanguageRI/Metal/MSLFunctionDeclaration.js:115
> > +        // We currently assuming that all parameters to entry points are attributes.
> > +        // TODO: Better logic for this, i.e. support samplers.
> 
> We have to do this before we present. Probably should file a bug.
> 

I’ve opened https://bugs.webkit.org/show_bug.cgi?id=189751, which covers both of these.

> > Tools/WebGPUShadingLanguageRI/Metal/MSLNativeFunctionCall.js:75
> > +        "native uint f32tof16(float)" : () => `${resultVariable} = uint(static_cast<ushort>(half(${args[0]})));`,
> 
> Pretty sure this shouldn't be a static_cast. It places the result "stored in
> the low-half of the uint."
> 
> https://docs.microsoft.com/en-us/windows/desktop/direct3dhlsl/f32tof16
> 

half -> float reduces the size to 16 bits, half -> ushort keeps the result in 16 bits, ushort -> uint places the 16 bits of the ushort in the lower 16 bits of the uint.

> > Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:85
> > +                emitter._zeroInitialize(node.baseType, variableName, false);
> 
> How does this work?
> enum Foo : int {
>     Bar = 1,
>     Baz = 2
> }
> Can't really zero-initialize that type, can we :(
> 

See https://bugs.webkit.org/show_bug.cgi?id=189755. I’ll fix it in that bug seen as this will affect the interpreter as well. No tests catch this at the moment.

> > Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:222
> > +        // At the moment the parser emits Block nodes for the if and else bodies, so this allows us
> 
> Really?

Nope, just checked.

> > Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:714
> > +            if (node.isLValue)
> > +                this._add(`${resultVar} = &(${bodyExpression});`);
> 
> I thought we were getting rid of this?
> 
> > Tools/WebGPUShadingLanguageRI/Metal/MSLStatementEmitter.js:722
> > +            if (node.isLValue)
> > +                this._add(`${resultVar} = &(${elseExpression});`);
> 
> Ditto

I’ve now removed the codegen for this, also see https://bugs.webkit.org/show_bug.cgi?id=189290.

> 
> > Tools/WebGPUShadingLanguageRI/Metal/MSLTypeNamer.js:86
> > +                name = `${node.addressSpace} ${typeNamer.mslTypeName(node.elementType)}*`;
> 
> Wouldn't it be easier to emit a collection of typedefs at the top of the MSL
> source for every used type in the entire program? You could do a prepass to
> populate it, so you wouldn't have to build up these long strings. It would
> work like how each WHLSL expression gets mapped to a MSL statement with
> exactly one operation - each used WHLSL type in the entire program would map
> to a MSL typedef with exactly one construction.
> 

This would work, although *this* code will still need to be there in order to generate the typedefs.

> > Tools/WebGPUShadingLanguageRI/Metal/MSLTypeNamer.js:91
> > +                name = `${node.addressSpace ? node.addressSpace + " " : ""}${typeNamer.mslTypeName(node.elementType)}*`;
> 
> Better to emit "thread" to be explicit.

Will do so long as there are no issues with Metal. At the moment I was avoiding this because in WHLSL only the outermost array type gets an address space, and that address space could be something other than thread.

> 
> > Tools/WebGPUShadingLanguageRI/Metal/MSLVarDeclaration.js:45
> > +    type = typeOf(type); // Unwrap TypeRef if necessary
> > +    if (type.isArray) {
> > +        let arrayType = type;
> > +        const sizes = [ arrayType.numElementsValue ];
> > +        let elementType = typeOf(arrayType.elementType);
> > +        while (elementType.isArray) {
> > +            arrayType = elementType;
> > +            sizes.push(arrayType.numElementsValue);
> > +            elementType = arrayType.elementType;
> > +        }
> > +        if (treatArrayAsPointer)
> > +            return `${arrayType.addressSpace ? arrayType.addressSpace : "thread"} ${typeNamer.mslTypeName(elementType)} (*${name})${sizes.slice(1).map(size => `[${size}]`).join('')}`;
> > +        else
> > +            return `${typeNamer.mslTypeName(elementType)} ${name}${sizes.map(size => `[${size}]`).join('')}`;
> > +    } else if (type.isPtr)
> > +        return (type).addressSpace + " " + mslVarDeclaration(typeNamer, (type).elementType, `(*${name})`);
> > +    else
> > +        return `${typeNamer.mslTypeName(type)} ${name}`;
> 
> If we typedef every used type, this logic could probably be much simpler.

Yeah. I like this idea.

> 
> > Tools/WebGPUShadingLanguageRI/Metal/TypeOf.js:27
> > +// FIXME: Rather than being a separate function this should instead be a separate preparation phase that annotates
> > +// each Node instance with a "type" property. https://bugs.webkit.org/show_bug.cgi?id=189611
> 
> Doesn't need to be a separate phase. Checker can do it.
> 
> > Tools/WebGPUShadingLanguageRI/Metal/TypeOf.js:47
> > +            return node.type.visit(this);
> 
> I don't understand why there are recursive calls on the .type properties.
> Why not just return node.type?

All the other code assumes that typeOf doesn’t return a TypeRef; the recursive call unwraps the TypeRef. Applies to all other methods here.

> 
> > Tools/WebGPUShadingLanguageRI/PropertyResolver.js:172
> > +        if (node.lValue.unifyNode instanceof DereferenceExpression)
> > +            node.become(new IdentityExpression(node.lValue.unifyNode.ptr));
> 
> Can you remind me again why this is necessary? This could potentially have
> side effects around lvalue validation, and we want to make sure our compiler
> matches the spec exactly. I don't think this kind of optimization should be
> made without making sure it's not observable.

I’ll add some tests to make sure this isn’t observable, although I currently can't think of a case where this is incorrect w.r.t the interpreter. Without this check, the codegen will generate the following for  &(*x):

int v0 = *x;
int* v1 = &v0;

However, this means that v1 != x, but it the interpreter/spec expect v1 == x (C works the same way). Therefore I collapse expressions of the form &(*x) to avoid the code generation being incorrect.

[Thomas Denney]

Created attachment 350174 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350174 [details]
Patch

Clearing flags on attachment: 350174

Committed r236299: <https://trac.webkit.org/changeset/236299>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Myles C. Maxfield]

Migrated to https://github.com/gpuweb/WHLSL/issues/109