Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

[WHLSL] Metal code generation #109

Closed
litherum opened this issue Oct 13, 2018 · 36 comments
Closed

[WHLSL] Metal code generation #109

litherum opened this issue Oct 13, 2018 · 36 comments
Milestone
m1

Comments

@litherum
Copy link
Contributor

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

At 2018-07-17T18:42:47Z, tdenney@apple.com wrote:
Adding WHLSL compiler
@litherum
Copy link
Contributor Author

At 2018-07-17T18:43:10Z, tdenney@apple.com wrote:
Created attachment 345174
Patch

@litherum
Copy link
Contributor Author

At 2018-07-17T18:45:29Z, ews@webkit.org wrote:
Attachment 345174 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.

@litherum
Copy link
Contributor Author

At 2018-07-17T20:25:55Z, ews@webkit.org wrote:
Comment on attachment 345174
Patch

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

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

@litherum
Copy link
Contributor Author

At 2018-07-17T20:26:06Z, ews@webkit.org wrote:
Created attachment 345185
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

@litherum
Copy link
Contributor Author

At 2018-07-17T21:24:11Z, dino@apple.com wrote:
Comment on attachment 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.

@litherum
Copy link
Contributor Author

At 2018-07-18T00:10:55Z, tdenney@apple.com wrote:
Created attachment 345208
Patch

@litherum
Copy link
Contributor Author

At 2018-07-18T00:14:49Z, ews@webkit.org wrote:
Attachment 345208 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.

@litherum
Copy link
Contributor Author

At 2018-07-18T02:28:40Z, tdenney@apple.com wrote:
Created attachment 345219
Patch

@litherum
Copy link
Contributor Author

At 2018-07-18T02:30:04Z, tdenney@apple.com wrote:
Created attachment 345220
Patch

@litherum
Copy link
Contributor Author

At 2018-08-28T03:06:19Z, tdenney@apple.com wrote:
Created attachment 348257
WIP

@litherum
Copy link
Contributor Author

At 2018-08-29T17:03:51Z, tdenney@apple.com wrote:
Created attachment 348407
WIP

@litherum
Copy link
Contributor Author

At 2018-09-01T03:22:53Z, tdenney@apple.com wrote:
Created attachment 348703
Patch

@litherum
Copy link
Contributor Author

At 2018-09-04T22:05:35Z, tdenney@apple.com wrote:
Created attachment 348852
Patch sans TypeScript

@litherum
Copy link
Contributor Author

At 2018-09-04T23:31:17Z, tdenney@apple.com wrote:
Created attachment 348869
Patch sans TypeScript

@litherum
Copy link
Contributor Author

At 2018-09-05T01:43:19Z, ews@webkit.org wrote:
Comment on attachment 348869
Patch sans TypeScript

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

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

@litherum
Copy link
Contributor Author

At 2018-09-05T01:43:21Z, ews@webkit.org wrote:
Created attachment 348884
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

@litherum
Copy link
Contributor Author

At 2018-09-06T23:57:22Z, mmaxfield@apple.com wrote:
Comment on attachment 348869
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.
      1. 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?

@litherum
Copy link
Contributor Author

At 2018-09-08T00:46:03Z, mmaxfield@apple.com wrote:
Comment on attachment 348869
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

@litherum
Copy link
Contributor Author

At 2018-09-08T00:52:52Z, tdenney@apple.com wrote:
(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.

@litherum
Copy link
Contributor Author

At 2018-09-08T02:21:03Z, mmaxfield@apple.com wrote:
(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.

👍

@litherum
Copy link
Contributor Author

At 2018-09-11T16:40:48Z, tdenney@apple.com wrote:
(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.

@litherum
Copy link
Contributor Author

At 2018-09-12T02:35:16Z, tdenney@apple.com wrote:
Created attachment 349505
WIP

@litherum
Copy link
Contributor Author

At 2018-09-12T06:02:55Z, ews@webkit.org wrote:
Comment on attachment 349505
WIP

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

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

@litherum
Copy link
Contributor Author

At 2018-09-12T06:02:57Z, ews@webkit.org wrote:
Created attachment 349524
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

@litherum
Copy link
Contributor Author

At 2018-09-12T20:57:03Z, tdenney@apple.com wrote:
(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.

@litherum
Copy link
Contributor Author

At 2018-09-13T01:52:23Z, webkit-bug-importer@group.apple.com wrote:
rdar://problem/44403031

@litherum
Copy link
Contributor Author

At 2018-09-13T02:10:43Z, tdenney@apple.com wrote:
Created attachment 349612
WIP

@litherum
Copy link
Contributor Author

At 2018-09-13T20:50:55Z, tdenney@apple.com wrote:
Created attachment 349696
WIP

@litherum
Copy link
Contributor Author

At 2018-09-14T02:37:58Z, tdenney@apple.com wrote:
(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.

@litherum
Copy link
Contributor Author

At 2018-09-14T04:49:13Z, tdenney@apple.com wrote:
Created attachment 349733
Patch

@litherum
Copy link
Contributor Author

At 2018-09-14T06:04:59Z, tdenney@apple.com wrote:
Created attachment 349735
Patch

@litherum
Copy link
Contributor Author

At 2018-09-14T22:19:59Z, tdenney@apple.com wrote:
Created attachment 349817
Patch

@litherum
Copy link
Contributor Author

At 2018-09-14T23:18:34Z, tdenney@apple.com wrote:
Created attachment 349826
Patch

@litherum
Copy link
Contributor Author

At 2018-09-15T00:40:14Z, ews@webkit.org wrote:
Comment on attachment 349826
Patch

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

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

@litherum
Copy link
Contributor Author

At 2018-09-15T00:40:16Z, ews@webkit.org wrote:
Created attachment 349839
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

@litherum
Copy link
Contributor Author

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

@litherum litherum added this to the m1 milestone Oct 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
m1
Development

No branches or pull requests
1 participant
@litherum